How best to handle Event Dispatchers during upgrade

As shown by #1449 (closed) and in PRs https://github.com/civicrm/civicrm-core/pull/13551 and https://github.com/civicrm/civicrm-core/pull/16034 there have been shown issues with dispatching most hooks when in upgrade mode, this ticket is to discuss the best ways forward and if we need to move the whitelisting or similar to the dispatcher or elsewhere ping @eileen @totten

I'm not sure if this is related, but we've observed that with the current upgrade process, all extension-defined permissions (hook_civicrm_permission) get lost (in Drupal7).

added git:civicrm-core triaged type:proposal + 1 deleted label

yeh that would be the place

so doing a quick audit

These hooks all go straight to the dispatcher and not through the invoke function

pre
post
unhandledException
eventDefs
alterResourceSettings
alterAngular
caseChange

I would also say @totten @eileen that we should add in the permission hook to the whitelisted ones as well i think that would be safe

@BjoernE what version - I think we tried to fix that & this is fall out from the fix

We first observed this rolling out 5.13.4, but we thought it was a fluke. We then saw it again with 5.19.3, and started looking into it. For the versions in between I don't have reliable data, sorry. If you want, we could patch it into 5.19.4 and tell you whether it worked....

@eileen i can't see it in this list https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Hook.php#L163 so i don't think it was fixed

@seamuslee so during upgrade the hook won't run - but the original issue is that during the upgrade a cached version of the available hooks was created that excluded any from ? extensions or drupal modules - I can't recall which - and then they continued to be missing from the cache until a cache clear

mentioned in commit e10329e6

mentioned in commit 80e74f7b

mentioned in commit 80e74f7b

mentioned in commit 2ac10e87

mentioned in commit 7544877f

mentioned in issue #1498 (closed)

mentioned in issue #1702 (closed)

mentioned in issue #1713 (closed)

@seamuslee @eileen I think we reduce the need for periodically updating the upgrade-list. See https://github.com/civicrm/civicrm-core/pull/17126

mentioned in commit 926d7afc

mentioned in commit c5fedcc3

mentioned in commit 36e8f226

mentioned in commit 29b6189c

mentioned in commit 1c3c3394

mentioned in commit ecb0ae5d

mentioned in commit e76ac979

Hi,

Thank you for raising an issue to help improve CiviCRM. As you may know, this issue has not had any activity for quite some time, so we have closed it.

We would like you to help us to determine if this issue should be re-opened:

• If this issue was reporting a bug, can you attempt to reproduce it on a latest version of CiviCRM?
• If this issue was proposing a new feature, can you verify if the feature proposal is still relevant? Did it get the concept-approved label? Have other people also shown interest? Could it be implemented as an extension?

If the answer to either question is yes, please feel free to comment or re-open the issue. Please also consider:

• Is it something that you could help implement, either by sending a patch or hiring someone who can?
• Would you be willing to fund the work, either through a CiviCRM partner, or the Core Team?

Thank you for your help and contributions to CiviCRM.

P.S. This is an automated message, see infra/gitlab issue 20. We understand that automatic responses are annoying, but given the number of open issues as the project evolves, we need a bit of help to triage and prioritise the most relevant issues.

closed