dev/core#2814 TokenCompatSubscriber - Evaluate tokens during "civi.token.eval" phase #21449
Conversation
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
compat
branch
2 times, most recently
from
f1d6cc9 to
fdb1fc9
Compare
eileenmcnaughton
changed the title
eileenmcnaughton
force-pushed
the
compat
branch
4 times, most recently
from
b25c254 to
c350856
Compare
| @@ -268,24 +320,11 @@ public function onEvaluate(TokenValueEvent $e) { | |||
| /** @var int $contactId */ | |||
| $contactId = $row->context['contactId']; | |||
| if (empty($row->context['contact'])) { | |||
| $contact = $this->getContact($contactId, $messageTokens['contact'] ?? [], TRUE); | |||
| $contact = $this->getContact($contactId, $messageTokens); | |||
There was a problem hiding this comment.
No longer loading 'all' here - just the 'needed' since 'all' is only needed if legacy tokens are in play & they do it
| @@ -615,15 +615,15 @@ protected static function sendReminderEmail($tokenRow, $schedule, $toContactID): | |||
| $mailParams = [ | |||
| 'groupName' => 'Scheduled Reminder Sender', | |||
| 'from' => self::pickFromEmail($schedule), | |||
| 'toName' => $tokenRow->context['contact']['display_name'], | |||
There was a problem hiding this comment.
This code was taking advantage of the fact the token compat subscriber was loading the full contact - I have switched to a 'pseudotoken' approach to 'direct' it to
There was a problem hiding this comment.
Ah, that's clever to tell it to fetch display_name. 👍
There was a problem hiding this comment.
I'm glad you think it's clever ....
eileenmcnaughton
force-pushed
the
compat
branch
from
c350856 to
8d11b99
Compare
|
@totten this is working now - with one issue - I'm not sure how to handle the fact some tokens return html. I think there is probably an easy answer but that is what the failing tests are highlighting |
eileenmcnaughton
force-pushed
the
compat
branch
from
8d11b99 to
d8b9f16
Compare
| @@ -382,7 +384,7 @@ public function testContactTokens(): void { | |||
| */ | |||
| public function hookTokenValues(array &$details): void { | |||
| foreach ($details as $index => $detail) { | |||
| $details[$index]['favourite_emoticon'] = 'emo'; | |||
| $details[$index]['important_stuff.favourite_emoticon'] = 'emo'; | |||
There was a problem hiding this comment.
@eileenmcnaughton Is this a contract change? On one hand, it smells sorta like it on the surface. OTOH, this part of the unit-test seems to be fairly recent, and there should be other places with additional coverage of the same hook. Is this a place where maybe the test was incomplete/passing-for-the-wrong-reason before?
There was a problem hiding this comment.
I think the test was wrong - I only just added it in the context of making this change but then when I made it pass & all the other token tests failed..... so I figured I had gotten it wrong
There was a problem hiding this comment.
Yeah & also here
There was a problem hiding this comment.
Cool. Yeah, I think that's right (the revision seems to match-up more with the devdocs and other tests).
totten
changed the title
|
@totten looks like that got it passing! |
|
Change looks good. This functionality has decent test coverage and passes. Merging. |
Overview
dev/core#2814 Fix tokenCompatSubscriber to conditionally evaluate legacy hook tokens
Before
legacy hook tokens evaluated in render
After
legacy hook tokens loaded in evaluate
Technical Details
As discussed the tokens should be loaded in evaluate and the token processor class should handle the rendering - but currently that is not how it is working. This adds a function to evaluate for legacy hooks that runs before the main contact evaluate function. It loads the contact with all 'returnProperties' if hooks are to be called - because that is what they expect....
But if we get as far as the main evaluate and the contact isn't loaded we just load the values we actually need for tokens
It is pretty hard finding the right size chunk for this - it would be sensible to ask why I didn't go that bit further and fix the contact tokens to follow the same pattern. OTOH this might be so big it just stalls. I did create #21450 in order to potentially address the 'too big' - if it seems to not go far enough then IMHO the solution is to see it as a process
Comments
Test cover is excellent on this code by now