Ensure that class-index is up-to-date after toggling extensions (A)

[totten]

Overview

After enabling or disabling an extension, you may end up with inconsistent (stale/incorrect) data in the Container. Fix that.

The problem arises because Container now depends on the class-index (ie ClassScanner's index cache), and the index is not cleared early enough.

Builds on #24712. Note that #24743 and #24744 are two similar alternatives. (Note: I've filled-in the description to reflect current understanding+commits. The discussion below starts during testing/drafting period.)

Before

Consider Eileen's test from #24712 -- this enables an extension (civiimport) which has a service-class (ImportSubscriber) and then expects that ImportSubscriber is fully loaded/operational.

The test was failing because the order of operations was (effectively):

1. Enable the extension
2. Rebuild the container
3. Clear the class index

Step 2 needs the current class-index, but it's stale - because it's not cleared until step 3. Consequently, the container is built with the old class-list. This gives us an inconsistent status -- because (eg) civiimport is partially active but not fully active.

After

The test from #24712 passes (with some tangential revisions on tearDown()).

The order of operations is (effectively):

1. Enable the extension
2. Clear the class index
3. Rebuild the container

Comments

• The class-index may still be cleared an extra time after enabling. But at least it will be correct.

• This patch affects the class-index cache (the summary of active classes across all active extensions). I have fingers-crossed that this won't affect test performance much -- because (a) the class-structure cache is still left in place and (b) the test was already clearing the class-index.

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

I think some of these test fails are Good Things™. The testfieldsHaveTitles one for example is easy to see is true:

civicrm-core/ext/afform/core/CRM/Afform/DAO/AfformSubmission.php

Lines 116 to 131 in c7c6911

	'id' => [
	'name' => 'id',
	'type' => CRM_Utils_Type::T_INT,
	'description' => E::ts('Unique Submission ID'),
	'required' => TRUE,
	'where' => 'civicrm_afform_submission.id',
	'table_name' => 'civicrm_afform_submission',
	'entity' => 'AfformSubmission',
	'bao' => 'CRM_Afform_DAO_AfformSubmission',
	'localizable' => 0,
	'html' => [
	'type' => 'Number',
	],
	'readonly' => TRUE,
	'add' => '5.41',
	],

Civi\Angular\ManagerTest.testGetModules
Civi\Angular\PartialSyntaxTest.testAllPartials
api_v3_ExtensionTest.testExtensionGetByStatus
CRM_Core_DAOConformanceTest.testFieldsHaveTitles
CRM_Dedupe_MergerTest.testGetCidRefs
CRM_Member_Form_MembershipRenewalTest.testSubmit
api\v4\Entity\ConformanceTest.testEntitiesProvider
api\v4\Entity\ManagedEntityTest.testManagedNavigationWeights

[eileenmcnaughton]

Interesting that this triggered those though

[demeritcowboy]

My thinking is that the CRM tests don't normally run with searchkit or afform installed, but by enabling civiimport in one of the tests it also enables searchkit and afform because of the <requires> line, and then it starts pointing out some things about those extensions because nothing disables them after the test.

[eileenmcnaughton]

@demeritcowboy ah that makes sense

[totten]

24762 should help the 'testFieldsHaveTitles'.

I've been poking around at Civi\Angular\PartialSyntaxTest.testAllPartials - but there's been a fair amount of drift (ie files that haven't been linted since forever).

Haven't really looked into any of the others.

[eileenmcnaughton]

I think we'd better fix the tearDown to disable those modules too. The fixed ones were good to get fixed but the dedupe one, for example, is not finding a real problem, just a data difference

[totten]

(@eileenmcnaughton) I think we'd better fix the tearDown to disable those modules too. The fixed ones were good to get fixed but the dedupe one, for example, is not finding a real problem, just a data difference

Yeah, that is overall easier than hunting through the various semi-problems that it showed (though some of these are things that should be fixed).

FWIW, I initially tried a rule of "the test should uninstall anything that it installed". But this brought some trouble because the order of disable/uninstall steps can be brittle when you have managed entities. So the latest commit just hard-codes the sequence.

Aside: Replicating the failures was a little cumbersome because it depends on the interaction between tests. This adhoc test-group seemed to work OK though. I saw it failing massively beforehand -- and passing afterward.

[demeritcowboy]

For completeness, and I promise this is the last time I'll mention it, the other option is don't do any class scanning and people can declare listeners. But, if people are ok with the tradeoffs, then +1 on the PR.

[totten]

@eileenmcnaughton @demeritcowboy - Tests are passing now. I've filled-in the description.

After doing the description, I do think this is consistent with reports of mysterious class-loading errors. The problem described/fixed here would only kick-in when (a) you enable or disable an extension and (b) the toggled extension relies on class-scanning. It would seem to clear-up as soon as you do an extra cache-clear.

I'm pretty tempted to switch this over to 5.55-rc.

[eileenmcnaughton]

I just rebased this on the rc since it is a regression (although one that most people would not hit it was introduced with new code in 5.54 & should be ported there).

I suspect I'll need to port the other fixes to get a pass

[eileenmcnaughton]

I think this is good. I'm not completely sure it will cure all of the oddness I've seen but I have confirmed today that the psr0 fix in afform etc fixes the other 'live' replicable variant of class loading I had going on here.

I will keep working with the code & try to dig into background processing & keep an eye out for anything that gets thrown up