[REF] Code readability changes on activity tokens. #17161
Conversation
I've been trying to get to grips with this code and this addresses 2 things I found very confusing - notably - using activityId when we were referring to a fieldname - we use a combination of & for variables but normally when it's closely related to a query we use activity_id - the function name getEntityContextSchema() was unclear to me - I concluded that in fact we were retrieving the nname of the field for the entity ID
|
(Standard links)
|
|
Token processor is currently only used via actionschedule, case PDF and (unmerged branch) to emailapi. So if we are going to change the format then now is a good time. We should do all schema keys though (ie. |
|
| @@ -53,7 +53,7 @@ public static function createDocument($activityIds, $html_message, $formValues) | |||
| $tp->addMessage('body_html', $html_message, 'text/html'); | |||
|
|
|||
| foreach ($activityIds as $activityId) { | |||
| $tp->addRow()->context('activityId', $activityId); | |||
| $tp->addRow()->context('activity_id', $activityId); | |||
| } | |||
| $tp->evaluate(); | |||
|
|
|||
There was a problem hiding this comment.
With a view to one day standardising more of this, how about:
$entityField = CRM_Activity_Tokens::getEntityIDFieldName();
foreach ($activityIds as $activityId) {
$tp->addRow()->context($entityField, $activityId);
}
There was a problem hiding this comment.
Hmm I commented below first so my comments may not make sense & also this comment supercedes the other..... If you look at my api PR I'm proposing removing this code inn favour of an api call
There was a problem hiding this comment.
The thought was that 'acitivity_id' or 'activityId' is defined once and avoids mismatch between these various uses. But that's all void if we replace it anyway.
| @@ -69,7 +69,7 @@ public static function createTokenProcessor() { | |||
| return new TokenProcessor(\Civi::dispatcher(), [ | |||
| 'controller' => get_class(), | |||
| 'smarty' => FALSE, | |||
| 'schema' => ['activityId'], | |||
| 'schema' => ['activity_id'], | |||
There was a problem hiding this comment.
As above, maybe:
'schema' => (array) CRM_Activity_Tokens::getEntityIDFieldName();
There was a problem hiding this comment.
So in this case I don't know it adds value - it's harder to read & it's still hard-coded in the sense that the class is hard-coded
|
@mattwire @aydun you are right that we don't have a clear standard - however we generally use $entityID for properties on classes but 'entity_id' for a field in an array. The row is kind of in between I guess but it feels a lot more like the latter since it's mostly populated by sql and it's an iterator class. Agree re contactId as well - I haven't figured out where it is being set as yet. |
Overview
Code readbility changes in 'inner workings' of token code
Before
Function name is ````getEntityContextSchema()```
activity_id key in the array is activityId
After
Function name is ````getEntityIDFieldName```
activity_id key in the array is activity_id
Technical Details
I've been trying to get to grips with this code - in part in the context of reviewing #16983 and this addresses 2 things I found very confusing - notably
variables but normally when it's closely related to a query we use activity_id
the name of the field for the entity ID
Comments
@mattwire @aydun putting this out there as more readable from my point of view (having battled a bit) before we got further