marketing/civicrm-website#163 Mitigate potential upgrade errors on missing msg_templates

[mlutfy]

Details: marketing/civicrm-website#163

Overview

civicrm.org had an upgrade error because the event_registration_receipt template (used by the Event Cart) is missing from the database.

This is probably some niche error on civicrm.org because of years of upgrades, so instead of looking deeper into the cause of the problem, I only added mitigation against missing templates.

I mean, if a core message template is missing, and it's used by the site, it's a more general bug than just an upgrade bug.

Before

Fatal error during upgrade.

After

Upgrade success.

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

[eileenmcnaughton]

@mlutfy wouldn't it be better to add the missing template?

[jusfreeman]

Just jumping in here to say that we've seen this problem at least 3 times in the past. In each case, it's a blocker to the DB upgrade completing. I am pretty sure that this has also caused the site to go off-line until the issue was resolved.

I do like this change because it handles the situation where the template does not exist in the database.

It would be nice if the missing template was added to the database automatically along with this detection (insert rather than skip the update) - if the PR could include that feature, then this would be great!

[kcristiano]

@eileenmcnaughton I think this fixes an existing problem and moves us forward. I agree it would be better to add back the template.

Could we separate this and merge this now and create an issue to check and add missing templates to the DB on upgrade? I am struggling to see a use case to delete the templates so I don't see any harm in adding back.

[mlutfy]

Edit: OK, if I understand correctly, the upgrader could detect a missing message template and add it back directly, just a few more lines of code. I'm just worried about going down that route because it requires more testing and should be done in a future release iteration, not a last-minute RC change?

Original message:

I understand it's related, and core upgrade code can't be responsible for corrupt data, but in general the upgrader tries to be robust and handle a few of those cases. I don't think this PR is unreasonable?~~

Detecting/fixing could be in an extension? or at least separate PR (when should detection/fixing happen?). It still seems like a lower priority to me.

[eileenmcnaughton]

@mlutfy it is lower priority - but changing the UPDATE to INSERT IGNORE would probably fix both cases

[mlutfy]

We still have to check for the IDs because otherwise the "INSERT IGNORE" part of the query still has WHERE id IN ()?

but we could add an 'else' clause to that condition which does an insert? but I think it's still missing some metadata in order to correctly insert it back? (title, subject, is_sms, etc)

I opened an issue to track that topic: https://lab.civicrm.org/dev/core/issues/1392

[eileenmcnaughton]

OK - I'll merge this & let you punt that part