Alternate to #24143 - Update all is_reserved message templates during upgrade and their corresponding is_default's if it hasn't been edited

[demeritcowboy]

Overview

Alternate to #24143.

Before

The is_reserved templates (and their corresponding unedited is_default templates) are only updated during upgrade if they're listed in the hardcoded list of ones that have changed.

After

All the is_reserved templates (and their corresponding unedited is_default templates) are always updated during upgrade, near the very end.

Technical Details

is_reserved is the db copy of the template distributed with core (as opposed to is_default which is the tpl that's in-use currently on the site). If the corresponding is_default has not been edited (i.e. is the same as the existing is_reserved before we update that to the latest), then it's safe to update that is_default too.

It's very convoluted how the flow goes with several different functions with very similar names. I should post my trace somewhere. For now, this doesn't change what happens with the hardcoded list, so for ones listed in there it will still do what it does, and then the is_reserved ones will get updated twice but other than some extra disk access that's ok, and I'm looking to improve that later.

Comments

There's more I'd like to do here but this is the minimum to meet the original goal of #24143.

The main thing is this doesn't fix the always-wrong preupgrade message. The determination of the preupgrade message is very tangled. There is some sense in having it computed at the same time as it updates the templates like now, but I think it would be better separated out, i.e. Just determine the message during the revisions loop, but then update the templates all at once after that.

[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).

[demeritcowboy]

Ooh now I broke it completely instead of just pre-5.26 - that must be worth points for something:

Summary
4.6.36-setupsh.sql.bz2	ERROR
4.7.31-setupsh.sql.bz2	ERROR
5.7.3-setupsh.mysql.bz2	ERROR
5.13.3-multilingual_af_bg_en.mysql.bz2	ERROR
5.13.5-setupsh.mysql.bz2	ERROR
5.21.2-setupsh.mysql.bz2	ERROR
5.27.4-setupsh.mysql.bz2	ERROR
5.33.2-setupsh.mysql.bz2	ERROR
5.39.1-setupsh.mysql.bz2	ERROR
5.45.3-setupsh.mysql.bz2	ERROR
5.47.2-tz.mysql.bz2	ERROR

It might help to know what these upgrade tests do exactly. If I upgrade locally it's fine.

[seamuslee001]

@demeritcowboy so basically it loads in a saved database found here https://github.com/civicrm/civicrm-upgrade-test and then run cv upgrade:db command to upgrade

[demeritcowboy]

Thanks.
Yeah the problem was I did something dumb, and it's a hidden warning in php 7 that happened to work out ok, but a fatal in php 8.

php -r "var_dump(version_compare(new StdClass(), '5.26', '<'));"

I should clean up the description since it looks like it's passing now.

[demeritcowboy]

Arrr, I'm missing something.

Situation: Online contribution is customized, event confirmation isn't. Suppose we don't list event confirmation in the hardcoded list since it's minor changes we don't need to tell people about.

5.54 - at the moment it's ok. It updates the is_reserved for online contribution, but also ONLY the is_reserved for event (as compared to if we had listed it in the hardcoded list where it would also update the uncustomized is_default).

5.55 - we now change the event tpl some more and list it in the hardcoded list. But now the fact that we updated is_reserved last time is a problem. It will think event is customized when it isn't, so won't update the is_default.

Let me take another look at #24143 but I'm not sure it can be done in that spot during the revisions loop, and as noted in the description here I think that needs to be untangled from determination of changed.

[eileenmcnaughton]

I had a concern reading this but looking at the code my concern is with the PR template, not the code - the function name rather answers my question updateReservedAndMaybeDefaultTemplates - but could we make it clearer above that uncustomised templates are ALSO updated

[demeritcowboy]

Ok I have updated. I misinterpreted your comment on the other PR but got myself straightened out.

[eileenmcnaughton]

I tested this by enabling message_admin & pulling in #24316 and #24321

and then did an initial preview on online receipts

& then ran upgrade with this and then retried

& rechecked and there was no change. So I checked the templates & there was no change and then, eventually, I realised I hadn't pulled down this PR - so am about to do that & repeat the above

[eileenmcnaughton]

Ah after my 'adjusted' testing process I see

and

So this looks fine on r-run

[eileenmcnaughton]

And re-testing with a changed template it didn't update

[eileenmcnaughton]

@demeritcowboy the only reservation I have about this is that the field workflow_id should be deprecated & avoided. We have to use it in the other upgrade script because some sites might be starting from a version where workflow_name has not yet been added.

Having said that, it's non blocking & we could merge this as is as a definite improvement & potentially fix to workflow_name as a follow up

[demeritcowboy]

Ok and thanks for testing. I'm looking to do another round at some point to fix the always-wrong message so could look at that id first. I had just copied the existing query.

[eileenmcnaughton]

Yeah the existing query has to work on older versions too -shall I merge this & if you do another round you can pick it up?

[demeritcowboy]

works for me

[eileenmcnaughton]

Cool - this is definitely a really good step in the right direction