php8.x fatal fix + bonus smarty consistency

[eileenmcnaughton]

Overview

Alternative to https://github.com/civicrm/civicrm-core/pull/27073/files

Before

the countable check that causes the problems is used to decide whether to assign the value to the template

After

The value is always assigned, as null if need be

Technical Details

Ensuring smarty vars are assigned regardless of the flow is one of our practices we are working towards

Comments

[civibot]

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

• ➡️ Demo site for this PR
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

[larssandergreen]

I'm checking for the corresponding conditionals in the message templates and it is confusing. For customPre and customPost, this looks fine. For additionalCustomPre and additionalCustomPost, this is less clear. Not sure I totally understand what's going on here, but here's what I have:

In the event_offline_receipt_html template, I don't see a conditional, but then it isn't clear to me if this part of the template is reachable. I think $customProfile is only assigned for an online event registration - we can't have additional participants for offline event registrations, can we? So maybe we can cut this out entirely.

For online registrations, it looks like we use buildCustomProfile, so I think we are OK as we don't actually use additionalCustomPre or Post directly, we only use them as part of $customProfile, so the assign here doesn't do anything. But maybe it would be clearer to only assign these when we actually use them (which as I understand it is just customPre and customPost, while $name can also be additionalCustomPre, additionalCustomPost or NULL). Otherwise, it's hard to understand when the variables are being assigned to the template for actual use and when we are just doing it because.

This is kind of a separate issue and I can do a separate PR if that makes sense.

[larssandergreen]

Actually, zooming out here: Is there any case when an offline event receipt contains profile fields at all?

[larssandergreen]

I'm working on a slightly more in-depth refactor for this (an alternative to your alternative, if you will). This will remove the pass by reference $template entirely, so will get rid of the source of the error (though CiviMobile uses the function, so I might need to leave $template in as a transitional measure).

[larssandergreen]

Jenkins test this please

[larssandergreen]

So if it doesn't make sense to do a deeper refactor, then I think this is ready to merge. I've given it an r-run (along with the original PR, agree that both should be merged).

@eileenmcnaughton can you think of any case where an offline receipt could possibly contain profile fields? If not, I'm going to remove those from the templates.

[eileenmcnaughton]

OK - I'll merge this based on your testing

Re whether the offline receipt could have template fields - for ? historical reasons the offline receipt has a custom fields block & the online receipts display the profiles that are available for the event - seems a bit odd to me but I have mostly focussed on cleaning up the code, rather than the UI

[pradpnayak]

Thanks @eileenmcnaughton