ManagedEntities - Allow targeted reconciliation. Add hook parameter. Simplify logic.

[colemanw]

Overview

Makes the ManagedEntity reconcile process more efficient and the code easier to read & understand.
Makes saving or reverting an Afform faster by only reconciling managed entities from that one module.

Before

• CRM_Core_ManagedEntities->reconcile() reconciles entities in all modules.
• hook_managed(&$entities) enumerates entities in all modules.

After

• CRM_Core_ManagedEntities->reconcile($modules = NULL) can optionally be limited to specific modules.

• hook_managed(&$entities, $modules = NULL) can optionally limit lookups to specific modules.

  All core extensions now support hook_managed with the $modules filter. Existing implementations of hook_managed are not required to abide by $modules, but doing so will improve performance. Example:

  function example_civicrm_managed(&$entities, ?array $modules = NULL) {
    if ($modules !== NULL && !in_array(E::LONG_NAME, $modules, TRUE)) {
      return;
    }
    // Otherwise, continue and add $entities.
  }

Technical Details

The CRM_Core_ManagedEntities class has been around for a while and gathered a bit of cruft in that time.
I found the code quite difficult to read and understand, as it was fetching and re-fetching records, sorting them and dividing them by module but ultimately discarding those divisions. Some records were getting processed twice. Here's what I did:

• Instead of fetching Managed records 3 times (for enabled, disabled and missing modules), now they are fetched only once and sorted by action (create, update, disable, delete).
• Instead of looping through each module and then processing some of its records (leaving gaps that have to be filled in later), loop through all the managed entities once and process them all.
• The reconcile() function now accepts a $moduleName param that will reconcile a single module, which fixes a fixme in Afform.
• Removed a lot of extra sorting that didn't serve any purpose.

[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 this FIXME has been staring me down for a long time. Finally fixed!

[totten]

Woot! That should help performance when editing afforms!

[colemanw]

@totten the prerequisite PR has been merged and I've rebased this one, so it's just the one commit now.

[totten]

jenkins, test this please

[totten]

So I did a reading on the code before-patch and after-patch, and I agree this looks better.

Just to verbalize... it appears to me (and this is post-hoc rationalization) that ManagedEntities.php has made a transition in style:

• In the original style, it had qualitatively distinct stages that targeted different situations (reconcileEnabledModules(), reconcileDisabledModules(), reconcileUnknownModules()). Each stage ran separately -- using its own techniques to find interesting records and act upon them.
• In the final style, it has a planner which builds a todo list. (ie loadManagedEntityActions() builds $managedActions.) Then it walks through the plan and executes each todo (reconcileEntities()).
• In the transition, it nominally used both styles. It first ran a general planner, and then it ran each of the stages -- and those stages cherry-picked todos from the plan (whichever todos seemed interesting to them). But this split makes it harder to sense the overall dataflow.

IMHO, the planner/todo-list approach is better than the 3-stage approach. eg I like that the planner is more function-y -- you get more ways to test+inspect the todo-list without modifying the DB. It may also be more amenable to re-sorting based on a dependency-graph.

This PR definitely makes it easier to read.

On r-code-style front, I'd be tempted to go a bit further. Personally, the names $managedActions and loadManagedEntityActions(): void are confusing. ($managedActions sounds equivalent to ScheduledJobs to me, and loadBlahBlahActions sounds like it's going to read an action-list from somewhere else. But it's really synthesizing the data.) I'd be more partial to $todos or $tasks or $plan or createPlan($declarations): array or buildTaskList($declarations): array. But that's bike-sheddy, and distinct from the other stuff in this PR.

There is a fair amount of test-coverage in CRM_Core_ManagedEntitiesTest and api\v4\Entity\ManagedEntityTest.

On the general conceptual level, I'm inclined toward merging.

On a more concrete QA level, there are also several smaller places where it twiddles types+names (eg DAO=>array) which I haven't really looked at. (I'm optimistic because those are mostly internal and because there's test-coverage. But I didn't have the general picture when I first skimmed them, and I should probably give them another read with this better sense of context.)

[colemanw]

@totten thanks for taking the time to get your head round this, I think you summed it up well, and I think you're right the before state of the class is transitional, and this PR basically completes the transition.
My reading of your conclusion is basically "this is good but could go further." If that's the case then I suggest we merge this & follow-up with another PR to finish renaming the variables, etc.

[eileenmcnaughton]

I also agree with the analysis - I think the intent of the transitional refactor was a step towards the plan & then do approach

[totten]

(Non-blocker/General observation) There's a tension around the lifecycle of $declarations, which has behavioral and stylistic dimensions:

• (Behaviorally) Are $declarations ephemeral things (loaded+destroyed each time you use ManagedEntities) or are they retained for a while (re-used in multiple calls to ManagedEntities)? How long should they live in memory?
• (Stylistically) Do we prefer to use more stateless design (eg cleanupDeclarations(array):array) or more stateful design (eg setDeclarations(array,string): void)?

A couple thoughtlets that stand out to me:

• There are two public operations relying on this data (reconcile() + revert()). Both of them rebuild the $declarations before doing their work. This behavior seems reasonable to me (though maybe that belief merits inspection). In any case, it appears consistent before+after patch.
• AFAICS, the $moduleName is never actually passed in to setDeclarations(). I was concerned it would mean some subtle corruption of $this->declarations. But it doesn't matter - b/c reconcile()+revert() will rebuild $this->declarations whenever they're called.
• It seems to me that (following the other changes) there's now less need to store data in $this. In the older design, it retained $this->declarations so that each reconcileFooModules() could do a pass through the list -- in the newer design, the planner does a single pass over $declarations.

Anyway, this comment has as many questions as answers, and these don't have to be resolved now. The relevant methods are protected functions in a de-facto final class, so we can continue to revise based on other analysis or discussion.

[colemanw]

@totten I've rebased this PR to include your suggestion about variable/function names, made $modules multivalued, tweaked and added your additional commit.

[totten]

@colemanw OK, cool. I'm flagging it "merge ready". I should give it some r-run (due diligence) since it's changed -- and if I do that today, then I'll merge it. (But if not, then it's probably fine as long as tests pass - and you can merge tomorrow.)

[totten]

Did some light r-run in a few areas. These all worked as expected:

• Running tools/mixin/bin/test-all
• (Afform) Toggling dashlet support, and then checking that the dashlets are added/removed on the dashboard
• (General) Go through lifecycle of installing/uninstalling various extensions that rely on managed data (esp CiviGrant and afform_admin)

Promoting to "merge on pass".

[colemanw]

Thanks @totten. Here's a followup because I realized that ManagedEntities->reconcile() is getting called from outside of core... because there's no API.

[colemanw]

@totten I have been pushing in the direction of ephemeral. A few versions ago I hit some bugs caused by them hanging around from one reconcile to the next and they were solved by the addition of this line.
I would be happy to push it further in that direction and make them local variables passed between functions instead of class variables.

[colemanw]

#23251

[totten]

Thanks @totten. Here's a followup because I realized that ManagedEntities->reconcile() is getting called from outside of core... because there's no API.

I assume that's a reference to #23243... Commented over there.

Pushed up a little more test-coverage here.

Expanded description to highlight changes to two interfaces (reconcile() and hook_managed).

Still merge on pass.