dev/core#4537 Ensure that Event Registration email works when CiviCon… #27237
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/4537 |
| @@ -56,7 +56,10 @@ public function setupSmartyAliases(TokenValueEvent $e) { | |||
| */ | |||
| public function onRender(TokenRenderEvent $e): void { | |||
| $useSmarty = !empty($e->context['smarty']); | |||
| $e->string = $e->getTokenProcessor()->visitTokens($e->string, function() { | |||
| $e->string = $e->getTokenProcessor()->visitTokens($e->string, function($token = NULL, $entity = NULL, $field = NULL, $filterParams = NULL) { | |||
There was a problem hiding this comment.
without CiviContribute there were issues in the event online receipt (and other related templates) because they would have tokens like
{contribution.tax_amount|boolean} in if statements and this was filtering that down to an empty string which then if there were multiple parts to the if e.g. {if $showTaxAmount && {contribution.tax_amount|boolean}} it was being passed to smarty as {if $showTaxAmount && } which of course is a PHP syntax error this causes it to resolve any that use boolean filter to be 0
There was a problem hiding this comment.
OK - let me see if I can move this into where booleans are otherwise rendered - ie this sort of area
civicrm-core/Civi/Token/TokenProcessor.php
Lines 494 to 499 in 1ce5c9b
There was a problem hiding this comment.
So the problem is we don't get to there because of
civicrm-core/Civi/Token/TokenProcessor.php
Line 381 in 1ce5c9b
There was a problem hiding this comment.
@seamuslee001 yeah we could do
Which feels more right to me - in that generally token filters are handled in the token system. OTOH I am somewhat terrified of the code in vistiTokens and associated functions cos it's hard to follow which might be a case for not touching it
There was a problem hiding this comment.
yeh that might be right but yeh likewise and it seemed like as this was doing the "cleanup" it might be the most sensible place to put things
There was a problem hiding this comment.
ok - well I'm otherwise happy with this PR so @totten can merge this as is or opt for the other location to handle unset booleans
There was a problem hiding this comment.
OK, this is the first time I've really looked at |boolean. So I'm trying to catch-up. Here's what I'm understanding:
- The idea in 6543dfe is to help write Smarty conditionals that key-off of token-data, eg
{if {contact.first_name|boolean}}...{/if}
- This kind of
{if {token}}notation is the quintessential quirk of Token-Smarty format. It's why Token-Smarty is evil and should never be used. It's also why we have to keep it around -- because changing to a simpler/singular language would break these conditionals. - So I guess the idea of
|booleanis to make the conditional a bit safer, eg{if {contact.first_name}}...{/if} {* More evil *} {if {contact.first_name|boolean}}...{/if} {* Less evil *}
- In the callpath of this specific bug, we have a CiviEvent message with expression:
{if $isShowTax && {contribution.tax_amount|boolean}} - In CiviEvent contexts, the token
{contribution.tax_amount}may or may not be defined. (CiviEvent is only softly dependent on CiviContribute.) - The approach here is to define
{any_unkn.own|boolean}to output0. - The question you're mulling is: Where, exactly, should we put in the override to make
{any_unkn.own|boolean}evaluate to0?
There was a problem hiding this comment.
So, yes, to my eye, both locations work, and the TokenProcessor::$getToken() seems a bit more poignant than TokenCompatSubscriber::onRender().
Whichever location it's in, I think it should have a code-comment explaining why it's there, e.g.
if ($filterParams && $filterParams[0] === 'boolean') {
// This token was missed during primary rendering, and it's supposed to be coerced to boolean.
// Treat an unknown token as false-y.
return 0;
}There was a problem hiding this comment.
@totten that seems about right
There was a problem hiding this comment.
Force-pushed a commit with the extra comment.
| 'end_date' => 20351023, | ||
| 'registration_end_date' => 20351015, | ||
| ]); | ||
| CRM_Core_BAO_ConfigSetting::disableComponent('CiviContribute'); |
There was a problem hiding this comment.
If I moved this before eventCreteUnpaid I ran into a whole bunch of issues with Example Data creation so decided to skip overall of that for now
8b32a77 to
8b28675
Compare
8b28675 to
f315f7a
Compare
|
Happy leap year CRM_Member_BAO_MembershipTest::testRenewMembership /home/homer/buildkit/build/build-2/web/sites/all/modules/civicrm/tests/phpunit/CRM/Member/BAO/MembershipTest.php:518 |
…tribute component is disabled and add unit test Shift to using action object provider and rename function to remove Component
f315f7a to
cf45130
Compare
…tribute component is disabled and add unit test
Overview
This fixes an issue where performing an event registration without the CiviContribute Component / Extension enabled errors because LineItem APIv4 class isn't present and also fixes issues with smarty template rendering in similar
Before
Event Registration fails without CiviContribute enabled
After
Works
ping @eileenmcnaughton @totten @MegaphoneJon