Simplify autogenerated boilerplate upgrader class

[colemanw]

Overview

De-clutters our autogenerated upgrader classes.

Before

Every month we autogenerate a file full of unnecessary boilerplate.

After

Simpler

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

[colemanw]

@totten I can't remember how to actually run this. Can you please r-run?

[totten]

It's generated by set-version.php whenever we bump up the middle number. If the current version is 5.46.alpha1, then you can bump up to 5.47.alpha1.

git scan am -N https://github.com/civicrm/civicrm-core/pull/22225
./tools/bin/scripts/set-version.php 5.47.alpha1 --no-commit
cv upgrade:db -vv

I can confirm that the above procedure still appears OK.

On the general merits... I am concerned that the signatures/lifecycles for the methods in CRM/Upgrade/Incremental/php/* are weird. One's gut sense for how to write these methods is often wrong/problematic. Not so weird as to confuse most readers when skimming existing methods -- but weird enough that it's very easy to get wrong when writing afresh.

OTOH, I think your point here is good wrt readers and overall readability - eg looking back over a list of 40+ of these classes, all the comments make it harder to focus-in on the things which are distinctive.

The concerns about writing could perhaps be mitigated by improving the docblocks in the base-class to include or link-to examples.

[colemanw]

Thanks. I was able to run it myself following that tip.

On the general merits... I am concerned that the signatures/lifecycles for the methods in CRM/Upgrade/Incremental/php/* are weird. One's gut sense for how to write these methods is often wrong/problematic. Not so weird as to confuse most readers when skimming existing methods -- but weird enough that it's very easy to get wrong when writing afresh.

Yea the upgrade classes are still a bit screwy.

OTOH, I think your point here is good wrt readers and overall readability - eg looking back over a list of 40+ of these classes, all the comments make it harder to focus-in on the things which are distinctive.

Not anymore! #22226 +86 −1,830!

The concerns about writing could perhaps be mitigated by improving the docblocks in the base-class to include or link-to examples.

I think the biggest gotcha is in the comment * Important! All upgrade functions MUST add a 'runSql' task. - that is indeed important but the previous boilerplate was messy-looking and kind of expected the comment to be deleted after being read, which was an invitation for merge-conflicts. I've reworked that into a docblock at the top of the class which is neater.

[demeritcowboy]

The if ($rev == part is also important, otherwise the pre/post message repeats (at least) 3 times (alpha/beta/.0), but not a blocker. It's inconsistent with the convention of upgrade_X function names having the version in the function name, so maybe one future approach would be to make that consistent, i.e. setPreUpgradeMessage_5.5.alpha1

[totten]

OTOH, I think your point here is good wrt readers and overall readability - eg looking back over a list of 40+ of these classes, all the comments make it harder to focus-in on the things which are distinctive.

(@colemanw) Not anymore! #22226 +86 −1,830!

😃

(@colemanw) I think the biggest gotcha is in the comment * Important! All upgrade functions MUST add a 'runSql' task.

(@demeritcowboy) The if ($rev == part is also important, otherwise the pre/post message repeats (at least) 3 times (alpha/beta/.0)...

Exactly.

I've pushed up some docblocks to replace the examples.

I'm OK with merge-on-pass here.