dev/core#2812 Fix issue where having a processor configured with a se…

[seamuslee001]

…arch output casues WSOD not allowing for access to upgrade screen

Overview

This fixes a problem with the dataprocessor extension being enabled after deploying the 5.41 code base and before upgrade step causing a WSOD due to the reliance on the legacy custom search classes

Before

1. Set up demo wordpress site
2. Install CIvi 5.40
3. Install Dataprocessor extension
4. create a sample dataprocessor that uses Contact as the source and has a field of Contact ID and outputs a Dashlet and a Search / Report.
5. Attach Dashlet on dashboard
6. Deploy the CIvi 5.41 codebase
   WSOD and cannot even access the UI upgrade screen

After

No WSOD

Technical Details

The problem seems to be that there is some heavy dependency on the base custom search class in dataprovider which is causing issues pre db upgrade step

ping @eileenmcnaughton @agileware-justin @totten

[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]

@seamuslee001 if we do this we should do the same for recaptcha I guess?

[demeritcowboy]

In the mini conversation at #20952 (comment) it was suggested extensions would need to "add the tag to their html", which I interpreted as "add dependencies to info.xml". Is that what's needed here?

[demeritcowboy]

Or maybe this is different since it happens earlier? I don't think the extension I tested did anything too fancy.

[seamuslee001]

@demeritcowboy I think the problem is that it is happening because they are referencing classes in the hook_container that depend on a class that has moved but the extension hasn't been installed by the upgrade step so the class cannot be found on the include path

@eileenmcnaughton I don't think we should do this for recaptcha because recaptcha I think is a different issue tbqh

[mattwire]

Is there a way to do this without having to hardcode things into CRM_Core_Classloader? Otherwise we potentially end up with a long list of hardcoded extensions when we only need them once (for the upgrade from "part of core" to "core extension"). It feels like there should be a way to handle this in the upgrader itself somehow?

[eileenmcnaughton]

@mattwire yeah I think all our brains are screaming about that one!

In this case not loading dashlets before the upgrade has run would prevent the symptoms as reported - and I suspect that would reduce other upgrade risks

Worth noting is that
a) we do need to get 'something' out there quickly but we can iterate and
b) if we do something like this we need to comment that it can be removed after x amount of time (give the bulk of the sites time to upgrade but don't carry a hack like that forever)

[demeritcowboy]

A tentative +1 on doing this for 5.42/5.41 and thinking about something better for master.

[totten]

Agree with the above comments - it's awkward as long-term fix, but it's OK as a short-term/temporary work-around.

The awkward aspects of this short-term fix are (1) unusual style+code-path (2) it will tend to obscure other ways that dependencies are misaligned. On MM, we discussed another potential short-term work-around: move CRM_Contact_Form_Search_Custom{,_Base} back to core-proper. But obviously it would undo some of the migration effort, so that also is just a short-term fix.

The main question seems to be: where should this land long-term? Some observations:

• There are ~30 other classes in contrib which reference CRM_Contact_Form_Search_Custom_Base.
• But these generally seem OK as-is. I spot-tested a couple, and upgrading worked fine. This makes sense - running the upgrader shouldn't require running those custom-searches. And once the upgrader is done, the custom-searches will work.
• There's something special about dataprocessor - it uses hook_config to call a static method CRM_DataprocessorSearch_Form_Search_Custom_DataprocessorSmartGroupIntegration::redirectCustomSearchToDataProcessorSearch. The failure arises because hook_config is trying to load "Custom Search" functionality unconditionally (at a time when it's unavailable).
• The functionality ofredirectCustomSearchToDataProcessorSearch() is to intercept requests for civicrm/contact/search/custom and redirect them to a different screen. It does not actually execute "Custom Search" functionality.

It seems to me that the goal would be:

• "Custom Search" continues moving into ext/legacycustomsearch, and it generally isn't required or used unless the extension is active.
• "Data Processor" continues supporting its redirect - but it should be more conservative (only load "Custom Search" artifacts if they are needed/applicable).

This MR for dataprocessor should make it safe on older+newer versions of core: https://lab.civicrm.org/extensions/dataprocessor/-/merge_requests/92

TBH, after understanding the bug better, I'm a little on the fence about whether civircm-core should have a mitigation - since I think the ultimate fix will be a patch to dataprocessor.git (and admins can mitigate without a patch - by disabling/re-enabling the extension). OTOH, this is an unexpected problem, and I think a short-term mitigation will significantly soften things so that fewer people get caught up in the error-case.

I'm +1 on merging the work-around for 5.41-stable - but ambivalent on adding it to 5.42-rc.

[eileenmcnaughton]

I would go with merging the released fix to the rc & then reverting in master - that is closer to our normal practice (normally we would merge to the rc first but it's otherwise the same) and also gives a little more time for the data processor fix to work it's way through the system.

[eileenmcnaughton]

On call agreed to merge & revert in master