dev/core#2748 Remove unused token assigns

[eileenmcnaughton]

Overview

Per https://lab.civicrm.org/dev/core/-/issues/2748 - this removes some no-longer-used token assigns & adds an upgrade message in case people have templates still referring to them

Before

We assign $first_name, $last_name, $displayName $contributeMode to the template but they have not been in the shipped templates for a long time - there is a preferred alternative (token) for the first 3

After

Assigns removed, system checks will warn

Technical Details

These variables are coming from the dreaded loadRelatedObjects function and in turn from loadRelatedEntitiesByID - which we hope to decommission

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

I did a clean build on 5.40 then edited the corresponding civicrm_msg_template to use the text:

<p>{$contributionMode} and {$last_name}</p>

Then made a snapshot and ran the upgrade using CLI and web UI:

This upgrade seems to create all the empty <li> bits.

The message about {$display_name} is pre-existing (from the 5.40=>5.41 upgrader). Tangentially, that message is a little confusing because you don't really have to do anything. (upgrade_5_41_alpha1() converts it.)

[eileenmcnaughton]

oh - @totten the intent with the message is that it is generated AFTER the upgrade in case any were missed - I guess I misunderstood the order of events?

[eileenmcnaughton]

@totten so if I just remove this bit is it right ? (Message wise - the face it is checking before rather than after is a separate & maybe not that solvable issue)

[totten]

Yeah, that emptiness check doesn't seem right. Arrays like [NULL,NULL,NULL] and ['','',''] are not empty(), but they are devoid of meaningful content. Maybe something like this...

$upgradeMessage = array_filter($upgradeMessage, function($v) { return !empty($v); });

... would trim the array down to only show meaningful content.

[totten]

oh - @totten the intent with the message is that it is generated AFTER the upgrade in case any were missed - I guess I misunderstood the order of events?

Yeah, I find that confusing too. Pulled it up in a debugger and also ran it in verbose mode (cv upgrade:db -vv).

So I think what happens is that it runs Upgrade DB to 5.41.alpha1 (CRM_Upgrade_Form::doIncrementalUpgradeStep...). As part of doIncrementalUpgradeStep, it calls both upgrade_5_41_alpha1() and setPostUpgradeMessage(..., 5.41.alpha1). Two things stand out:

1. (More important) upgrade_5_41_alpha1() only enqueues the task -- it doesn't actually execute the change yet. So when you get to setPostUpgradeMessage(), the change hasn't taken effect yet. That remains for a future task.
2. (Less important) setPostUpgradeMessage() can be called multiple times, once for each incremental version (alpha1, alpha2, beta1, beta2, .0, .1, etc). Pretty much everything that occurs within setPostUpgradeMessage() should have a guard so that it only runs once (e.g. if ($rev === '5.41.alpha1') { generateSomeWarnings();}).

[eileenmcnaughton]

@totten I've ripped out the html (I possibly went too far but it took @demeritcowboy about 4 commits to get tit right last time :-) & made the message a bit more accurate.

I also added a version check.

But to go the extra step & only provide the message if it might apply seems like a bigger challenge?

[eileenmcnaughton]

or rather - only display the message if it might STILL apply after the upgrade has run

[eileenmcnaughton]

@totten I've updated this - I an on the fence about adding a conditional warning to check the status page - or just relying on it (that could be a follow up) - I copied the 'name' pattern from Dr when & I suspect that I'd need to revist to be able to call the check usefully from the upgrade script

I'm inclined to leave it for these tokens as it's been quite a while since they were used

[eileenmcnaughton]

@totten I think we can merge this now? It is now using the status check mechanism to warn. These variables have not been used in core templates for a long time so as a minor cleanup we should be good on this one.

(now that we have a mapping between smarty & tplparams we have another tool for this stuff - but this is a complex bit of code so I think it's stil worth doing this cleanup - especially since it is so long since we shipped templatest that used these)

[demeritcowboy]

This has conflicts now.

[eileenmcnaughton]

@demeritcowboy thanks - the upgrade script not surprisingly - fixed now

[demeritcowboy]

This should be $contributeMode

[eileenmcnaughton]

ah thanks - fixed!

[demeritcowboy]

Since this went through some iterations, just to confirm:

• I shouldn't be expecting any post-upgrade messages about smarty vars it replaced, right? Just a status check? (It did NOT give me a post-upgrade message.)

[eileenmcnaughton]

@demeritcowboy that is correct - ideally no action is required but if there were some more complex smarty stuff going on it might be

[demeritcowboy]

Ok I think that makes sense here - it can't really replace arbitrary smarty like {if $last_name eq 'foo'} by itself anyway. So looks good.

[eileenmcnaughton]

thanks @demeritcowboy - nice to get rid of that contributeMode !