dev/core#3722 Stop infinite reconciliation #23955

herbdool · 2022-07-06T15:48:01Z

Overview

Fixes https://lab.civicrm.org/dev/core/-/issues/3722

Before

It schedules managed entity reconciliation over and over and over and over and over again.

After

It only schedules reconciliation if the definition is from an xml file and it is in the db already.

Technical Details

New to 5.50.0 we can now export CaseTypes as managed entities from API4. This attempts to fix the assumptions in CaseType which thinks it's coming from an xml file alone.

civibot · 2022-07-06T15:48:07Z

(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 · 2022-07-06T16:14:46Z

I'm unclear what this reconciliation is even for. Why is it needed? And why is it needed only under that condition?
Some code comments might help.

CRM/Case/BAO/CaseType.php

herbdool · 2022-07-06T17:43:39Z

@colemanw I'm not sure if we can remove the reconciliation altogether, but that would be the cleanest option. I can only imagine it's there because of possible "forked" entities? It only gets called if the definition parameter is set which in turn would not be set if all these conditions apply: editing a case type, is not yet forked, and is not forkable. Or to be clearer reconciliation will take place if Case Type is forked, or new, or is forkable.

herbdool · 2022-07-06T17:50:58Z

@colemanw here's where it was added 9c19292 "CRM-14786 - CaseType - Perform reconciliation whenever a case-type definition changes" https://issues.civicrm.org/jira/browse/CRM-14786

Sounds like it is only specific to the XML file where adding an would automatically create the activity type if it doesn't already exist. I assume we don't want that with mgd.php files which should be explicit for all of those. And doesn't play nicely with that anyway.

herbdool · 2022-07-06T17:59:40Z

I added some code comments.

herbdool · 2022-07-06T20:25:40Z

Looks like my PR isn't backwards-compatible yet.

demeritcowboy · 2022-07-06T20:31:18Z

I was wondering what would happen if you call api casetype.create with a definition. Then there's no physical xml file, so it looks like it's skipping the reconciliation. That's how I interpret the test fail.

herbdool · 2022-07-06T20:52:46Z

@demeritcowboy that seems spot on. In the test it's created it with a fixture, just defined in a variable. It doesn't seem like quite a real world test of functionality but I guess it's possible. Now I'm not sure what to check for. Perhaps a parameter that's only found in the API4 export. The one that seems obvious is 'version' => 4 but that isn't included in the parameters passed to CRM_Case_BAO_CaseType::add().

herbdool · 2022-07-06T23:48:44Z

Going to see if it's okay with specifying 'version' => 3.

demeritcowboy · 2022-07-07T00:00:28Z

But then what if you call \Civi\Api4\CaseType::create()->addValue('definition', ...) ? That would fail to do its thing.

Ideas:

add a \Civi::$statics in

civicrm-core/CRM/Core/ManagedEntities.php

Line 47 in 4ade779

public static function scheduleReconciliation() {

so it only does it once, or
Store a hash of the contents of each reconciled thing in

civicrm-core/CRM/Core/ManagedEntities.php

Line 531 in 4ade779

protected function createPlan(array $declarations, $modules = NULL): array {

, or somewhere in that file, and don't reconcile two things with the same hash twice.

totten · 2022-07-07T04:43:10Z

Comment: Basing the behavior on v3-vs-v4 makes more sense than the prior draft that used xml-file-presence.
GUI

In the test it's created it with a fixture, just defined in a variable It doesn't seem like quite a real world test of functionality but I guess it's possible.

FWIW, IIRC, it was the real-world scenario when this API+GUI+test were introduced. The GUI simply called CaseType.create API and passed in the definition, and the API would do any maintenance updates on ActivityTypes, RelationshipTypes, etc. The GUI was literally just an editor for the definition (which was the source of truth).

The GUI changed somewhere along the line (it now opens various quickform popups), so yeah - it probably doesn't rely on that behavior so much.
Auto installation

With the recent PR that updated hook_managed to support CaseType@v4, I recall being surprised that it worked - and learning that it worked because it relied on autocreating items from the definition. There are a few code-paths to autocreation, and this PR may or may not be impact each. I think the main thing is to test and not leave it up to chance.

I posted a couple related PRs with tests: (NFC) case-xml@1 - Add example+assertions of new activity-type #23959 and (NFC) mgd-php@1 - Add example+assertions for new case-type #23961.

Unfortunately, while checking for baseline (master, pre-PR), I found that it hook_managed[CaseType] seems to crash if you use an actual/exported CaseType with a real definition. (demonstration, Gitlab issue)

herbdool · 2022-07-07T11:43:29Z

But then what if you call \Civi\Api4\CaseType::create()->addValue('definition', ...) ? That would fail to do its thing.

@demeritcowboy right. I just tested and yes, it won't reconcile when adding a case type that way.

I might try setting a statics as you suggest.

herbdool · 2022-07-07T12:05:05Z

When I try the equivalent of \Civi\Api4\CaseType::create()->addValue('definition', ...) via the API4 GUI it encodes all the XML tags so it gets stored incorrectly. Not sure if that also happens if doing it directly in code.

As for statics, this seems to work:

     if (!isset(\Civi::$statics[__CLASS__][$params['name']])) {
        \Civi::$statics[__CLASS__][$params['name']] = 1;
        CRM_Core_ManagedEntities::scheduleReconciliation();
      }

Though you suggested putting it in CRM_Core_ManagedEntities so I'll try it there as well.

herbdool · 2022-07-07T12:13:22Z

This works too:

  public function reconcile($modules = NULL) {
    $modules = $modules ? (array) $modules : NULL;
    $declarations = $this->getDeclarations($modules);
    $plan = $this->createPlan($declarations, $modules);
    if (!isset(\Civi::$statics[__CLASS__]['plan'])) {
      \Civi::$statics[__CLASS__]['plan'] = TRUE;
      $this->reconcileEntities($plan);
    }
  }

herbdool · 2022-07-07T14:33:49Z

Now I'm trying a static. I ended up putting it into CRM_Case_BAO_CaseType::add() because I figured here we can ensure it gets called at least once for each case type.

demeritcowboy · 2022-07-07T16:01:46Z

via the API4 GUI it encodes all the XML tags so it gets stored incorrectly

Are you passing a string? It's expecting an array, at least in v3, like in that test that failed earlier:

civicrm-core/tests/phpunit/api/v3/CaseTypeTest.php

Line 27 in eaf24b5

'definition' => [

. It's maybe a little odd it even accepts a string.

demeritcowboy · 2022-07-07T16:08:17Z

CRM/Case/BAO/CaseType.php

+      // @see CRM-14786 Reconcilation added to manage whenever a case-type XML definition changes,
+      // in order to add/remove associated activity and relationship types.
+      if (!isset(\Civi::$statics[__CLASS__][$params['name']])) {
+        \Civi::$statics[__CLASS__][$params['name']] = 1;


$params['name'] can be blank, e.g. if calling this to update using id instead of name. There is $caseTypeName which should always be set. But I'm wondering if it even needs to be that specific, since (I think) the reconciliation will reconcile whatever needs reconciling and so just needs to be called once. To be honest I'm not sure exactly how it works, and when PHASE_POST_COMMIT occurs.

In reverse, I suppose there's also the situation where, e.g. during upgrade, this might be an issue, because let's say you have something that does casetype.create in 5.52 and something calls reconciliation, then another casetype.create in 5.53 and something calls reconciliation. That second one won't run if running upgrades via cli since the static will stick.

I'm just thinking of some things to test out.

Yeah I meant to use $caseTypeName.

Based on your second thought it seems like setting it per $caseTypeName might be needed. Unless even that could be repeated across upgrades.

herbdool · 2022-07-07T16:28:58Z

Are you passing a string?

Yes. I blindly tested it in the GUI. I'm assuming that in the GUI there's no way to pass it an array. So it seems a bit of an oddball.

herbdool · 2022-07-07T22:49:14Z

I can't figure out the failure. Says there were no test failures.

demeritcowboy · 2022-07-07T23:10:34Z

When that happens you need to hunt in the console log:

1) E2E_Shimmy_LifecycleTest::testLifecycleWithSubprocesses
RuntimeException: Command failed (cd '/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin'; cv api3 --in=json 'Extension.enable'):
{"error_code":"already exists","tip":"add debug=1 to your API call to have more info about the error","is_error":1,"error_message":"DB Error: already exists"}


/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:161
/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:119
/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:72
/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:43
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

2) E2E_Shimmy_LifecycleTest::testLifecycleWithLocalFunctions
CRM_Core_Exception: DB Error: already exists

/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/all/modules/civicrm/api/api.php:135
/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:104
/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:68
/home/jenkins/bknix-dfl/build/core-23955-56ru3/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:54
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

herbdool · 2022-07-08T15:58:34Z

Running out of steam here. So the last version of this PR that passed as where I restricted it to v3, though not sure if it would pass against master now.

demeritcowboy · 2022-07-08T16:16:52Z

Taking a step back, what is it you're ultimately trying to do in your extension? Can you just implement hook_civicrm_caseType and put your case type in an xml file?

herbdool · 2022-07-08T16:33:39Z

For my own project I can just patch Civi via composer until this gets fixed.

I had an xml file up until recently. I had also specified some activity types in mgd.php files. But when I added another activity type in mgd.php and also specified it in the casetype xml file CiviCRM complained that the activity type already existed. It was only when I switched to just using mgd.php that it worked.

So with a patch I can wait a bit - I don't have to worry about all the permutations on my own project.

herbdool · 2022-07-12T21:42:42Z

In case it turns out that checking if it's API4 or API3 managed entity is the best way to go, then I just came across !CRM_Core_BAO_Managed::isApi4ManagedType($item['entity_type']) in CRM_Core_ManagedEntities.

colemanw · 2022-07-13T01:31:29Z

In case it turns out that checking if it's API4 or API3 managed entity is the best way to go, then I just came across !CRM_Core_BAO_Managed::isApi4ManagedType($item['entity_type']) in CRM_Core_ManagedEntities.

That function checks if an entity is supported by v4, not whether a particular managed record uses v3 or 4. But that is possible to check as well by inspecting a given record. Something like if (($record['params']['version'] ?? 3) == 4)

demeritcowboy · 2022-07-13T13:41:36Z

That was effectively the same as one of the earlier commits, it's just that then calling \Civi\Api4\CaseType::create()->addValue('definition', ...) doesn't do its thing on its own, which is maybe ok but docs should be added somewhere (this link doesn't seem the right place https://docs.civicrm.org/dev/en/latest/api/v4/differences-with-v3/, so maybe https://docs.civicrm.org/dev/en/latest/api/v4/changes/).

There might be some bugs creep in if v3 calls get blindly replaced with v4 calls - but there's only one obvious place I see in core and it's unlikely to be replaced (in an upgrade script).

herbdool · 2022-07-13T14:55:45Z

@demeritcowboy does this mean you're warming up to the idea of checking v3/v4 so long as it's documented not to try \Civi\Api4\CaseType::create()->addValue('definition', ...)? If so I can try 3cb237c again now that we've got new tests.

demeritcowboy · 2022-07-13T20:44:05Z

Warming might be too strong a word (grin) but it looks like there's very few obvious uses in universe, and we know there's at least one test that covers the v3 use, so go for it!

herbdool · 2022-07-14T02:47:15Z

Gr... I don't even see the test civicrm/tests/phpunit/api/v4/Custom/FalseNotEqualsZeroTest.php which is failing. Does it exist?

demeritcowboy · 2022-07-14T03:08:56Z

1) E2E_Shimmy_LifecycleTest::testLifecycleWithLocalFunctions
CRM_Core_Exception: Cannot uninstall extension; disable it first: shimmy

/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/all/modules/civicrm/api/api.php:135
/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:104
/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:69
/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:54


2) E2E_Shimmy_LifecycleTest::testLifecycleWithSubprocesses
ActivityType "Nibble" should be auto enabled. It's missing.
Failed asserting that null matches expected 'Nibble'.

/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/default/files/civicrm/ext/example-mixin/tests/mixin/ManagedCaseTypeTest.php:30
/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:96
/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:73
/home/jenkins/bknix-dfl/build/core-23955-5hzs5/web/sites/default/files/civicrm/ext/example-mixin/tests/phpunit/E2E/Shimmy/LifecycleTest.php:43

civicrm-builder · 2023-01-15T07:28:21Z

Can one of the admins verify this patch?

colemanw · 2023-09-15T20:02:19Z

@herbdool thanks for your work on this. I was able to resurrect this PR in the form of #27430 - I used a slightly different approach to prevent recursion (the new-ish ability of managed entities to target a specific module) and that stopped the test from crashing... then I found out that the test itself was broken. That was a fun one.

herbdool · 2023-09-16T12:35:31Z

Good to see this gnarly one fixed!

Only reconcile if has an xml file

3e6e67d

civibot bot added the master label Jul 6, 2022

herbdool changed the title ~~Only reconcile if has an xml file~~ dev/core#3722 Only reconcile if has an xml file Jul 6, 2022

herbdool changed the title ~~dev/core#3722 Only reconcile if has an xml file~~ dev/core#3722 Only reconcile if there is an xml file Jul 6, 2022

demeritcowboy reviewed Jul 6, 2022

View reviewed changes

CRM/Case/BAO/CaseType.php Show resolved Hide resolved

herbdool added 2 commits July 6, 2022 12:43

New method that checks if it has an xml definition

65d7003

Update CaseType.php

bc55c8e

Update CaseType.php

2d56e8e

Update CaseType.php

3cb237c

herbdool changed the title ~~dev/core#3722 Only reconcile if there is an xml file~~ dev/core#3722 Stop infinite reconciliation Jul 7, 2022

herbdool added 2 commits July 7, 2022 10:28

Merge branch 'civicrm:master' into core-3722

0aefef7

Update CaseType.php

9c073eb

colemanw mentioned this pull request Jul 7, 2022

(NFC) mgd-php@1 - Add example+assertions for new case-type #23961

Merged

herbdool added 2 commits July 7, 2022 10:36

Enable checking activity type

2a1f07a

Update BunnyDance.mgd.php

53e72ff

demeritcowboy reviewed Jul 7, 2022

View reviewed changes

Update CaseType.php

a56b91f

Try check API version again - with new tests

e409180

colemanw mentioned this pull request Sep 15, 2023

Resurrect Bunny Dance #27430

Merged

colemanw closed this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#3722 Stop infinite reconciliation #23955

dev/core#3722 Stop infinite reconciliation #23955

herbdool commented Jul 6, 2022

civibot bot commented Jul 6, 2022

colemanw commented Jul 6, 2022

herbdool commented Jul 6, 2022

herbdool commented Jul 6, 2022 •

edited

Loading

herbdool commented Jul 6, 2022

herbdool commented Jul 6, 2022

demeritcowboy commented Jul 6, 2022

herbdool commented Jul 6, 2022

herbdool commented Jul 6, 2022

demeritcowboy commented Jul 7, 2022

totten commented Jul 7, 2022 •

edited

Loading

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

demeritcowboy commented Jul 7, 2022

demeritcowboy Jul 7, 2022

herbdool Jul 7, 2022

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

demeritcowboy commented Jul 7, 2022

herbdool commented Jul 8, 2022

demeritcowboy commented Jul 8, 2022

herbdool commented Jul 8, 2022

herbdool commented Jul 12, 2022

colemanw commented Jul 13, 2022

demeritcowboy commented Jul 13, 2022

herbdool commented Jul 13, 2022

demeritcowboy commented Jul 13, 2022

herbdool commented Jul 14, 2022

demeritcowboy commented Jul 14, 2022

civicrm-builder commented Jan 15, 2023

colemanw commented Sep 15, 2023

herbdool commented Sep 16, 2023

dev/core#3722 Stop infinite reconciliation #23955

dev/core#3722 Stop infinite reconciliation #23955

Conversation

herbdool commented Jul 6, 2022

Overview

Before

After

Technical Details

civibot bot commented Jul 6, 2022

colemanw commented Jul 6, 2022

herbdool commented Jul 6, 2022

herbdool commented Jul 6, 2022 • edited Loading

herbdool commented Jul 6, 2022

herbdool commented Jul 6, 2022

demeritcowboy commented Jul 6, 2022

herbdool commented Jul 6, 2022

herbdool commented Jul 6, 2022

demeritcowboy commented Jul 7, 2022

totten commented Jul 7, 2022 • edited Loading

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

demeritcowboy commented Jul 7, 2022

demeritcowboy Jul 7, 2022

Choose a reason for hiding this comment

herbdool Jul 7, 2022

Choose a reason for hiding this comment

herbdool commented Jul 7, 2022

herbdool commented Jul 7, 2022

demeritcowboy commented Jul 7, 2022

herbdool commented Jul 8, 2022

demeritcowboy commented Jul 8, 2022

herbdool commented Jul 8, 2022

herbdool commented Jul 12, 2022

colemanw commented Jul 13, 2022

demeritcowboy commented Jul 13, 2022

herbdool commented Jul 13, 2022

demeritcowboy commented Jul 13, 2022

herbdool commented Jul 14, 2022

demeritcowboy commented Jul 14, 2022

civicrm-builder commented Jan 15, 2023

colemanw commented Sep 15, 2023

herbdool commented Sep 16, 2023

herbdool commented Jul 6, 2022 •

edited

Loading

totten commented Jul 7, 2022 •

edited

Loading