dev/core#5493 use ClassScanner to find Api4 entities in ActionObjectProvider #31198

ufundo · 2024-09-27T14:19:35Z

Overview

Attempt 2 to find a way to fix https://lab.civicrm.org/dev/core/-/issues/5493

Before

during upgrade, Api4 classes are available for extension entities, but the underlying Entities are not in the EntityRepository
strange crashes ensue when Api4 metadata is gathered: like this one on Standalone upgrade test https://lab.civicrm.org/dev/core/-/issues/5493

After

Api4 entities are loaded using ClassScanner - which loads extension classes through scanClasses - so also respects the upgraders dispatch policy
Api4 entities from extensions won't be available during the upgrader. Added a (better) opt out to CRM_Core_Permission_Standalone where this was causing an issue
🤞 Standalone upgrade test passes

civibot · 2024-09-27T14:19:39Z

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

ufundo · 2024-09-27T14:21:35Z

Civi/Api4/Provider/ActionObjectProvider.php

@@ -48,6 +50,10 @@ public static function getSubscribedEvents() {
  public function onApiResolve(ResolveEvent $event) {
    $apiRequest = $event->getApiRequest();
    if ($apiRequest instanceof AbstractAction) {
+      if (!isset($this->getEntities()[$apiRequest->getEntityName()])) {


This gives a slightly more informative warning if the mismatch occurs. You get Api not implemented rather than DB table does not exist.

Possibly we could throw the error right here - I wasn't sure if there are legitimate cases where something else might provide the Api.

Nope, that's it. That function invokes 'civi.api4.entityTypes' which is the only way to add extra entities, so this is good.

Changed this from a log message to an exception and... it "fixes" a bunch of fails in the upgrade test, because Exceptions get swallowed e.g. when reconciling ManagedEntities

Which isn't totally satisfactory, but I guess maybe why it's not failing more currently

ufundo · 2024-09-27T14:52:34Z

good = upgrade test passes on Mysql8!
irrelevant = upgrade test fails on Mysql 5.7 (due to sql snapshot version, open pr on civicrm-upgrade-test)
bad = lots of other tests seem to hit different mismatches :/

Civi/Api4/Generic/AbstractEntityInterface.php

colemanw · 2024-09-27T14:59:23Z

Civi/Api4/Generic/AbstractEntityInterface.php

+ */
+interface AbstractEntityInterface {
+
+  // TODO: flesh out the interface


Basically all the public functions in AbstractEntity should go here.

ufundo · 2024-09-30T10:02:43Z

So my read of the new test fails:

1. OAuth doesn't currently have scan-classes mixin, so its Api4 classes weren't picked up under the new model.

Makes me realise that this change wouldn't be totally backward compatible -- if you have an extension with Api4 entity classes you now require scan-classes for those to get picked up.

To me this seems reasonable semantically (basically the old getAllApi4Classes is doing some class scanning even without scan-classes). Though I understand there can be performance implications for scan-classes if you have a complex extension - so maybe a lighter alternative that only scans the Api4 classes? We could also add a core listener to hook_civicrm_scanClasses that replicates the scanning that is currently happening by default?

2. Mock Api4 entities in the tests not getting picked up by class scanner

Have amended ClassScanner to fix this.

3. There's an upgrade step that seems to use ParticipantStatusType (not explicitly that I can see, but somewhere under the hood).

This causes a problem because under that entity now lives in civi_event extension, and now the extension entities aren't loaded during upgrade. Not sure what to do about that. I think the PR is making more explicit an issue that will already exist with this being a bit unstable?

ufundo · 2024-09-30T14:54:51Z

Two new fails that look relatively easy fixes:

CRM_Financial_Form_PaymentFormsTest::testEventPaymentForms - https://test.civicrm.org/job/CiviCRM-Test/27404/testReport/
EventCartParticipant.get not available. That entity should come from the eventcart extension, so my guess would be cache isn't getting regenerated right when the extension is installed in the setUp.

api\v4\Entity\ConformanceTest::testConformance with data set "MockBasicEntity" ('MockBasicEntity') - https://test.civicrm.org/job/CiviCRM-Test/27400/testReport/
The Mock entities are now subject to ConformanceTest - prob just a bit of missing metadata.

But not sure what to do about the main upgrade test fails. Some upgrade steps are calling Apis that are (now) provided by extensions, but the upgrade dispatch policy means extensions aren't "all there" during the main part of the upgrade. It feels like these upgrade steps become extension upgrade steps - but that creates a resequencing issue. Some way to explicitly allow those hooks / apis temporarily for just the upgrade steps that require them?

colemanw · 2024-10-01T15:39:51Z

CRM/Core/Permission/Standalone.php

@@ -89,7 +90,8 @@ public function upgradePermissions($permissions) {
    if (empty($permissions)) {
      throw new CRM_Core_Exception("Cannot upgrade permissions: permission list missing");
    }
-    if (class_exists(Role::class)) {
+    // sometimes during upgrade the extension entities aren't available
+    if (CoreUtil::entityExists('User')) {


Well that sucks

ufundo · 2024-10-04T08:25:39Z

Sorry this isnt really Ready for Review. I will work on it some more today but would appreciate your 2 cents if you have a minute @colemanw or @totten

From my investigation I think the top line is "there is inconsistency in how the dispatch policy during upgrade impacts what low level things are brought in from extensions".

It started with a mismatch between entityrepository entities (not loaded from ext) and api entities. I've now found a case it seems to extend to SpecProviders.

Looking at the LegacySpecProvider - I'm thinking we essentially need to do something similar with how Api4 entities are loaded from extensions (namely: make it go through scan-clases, but probably add a LegacyEntityProvider that doesn't require scan-classes, but does respect the dispatch policy).

(I also think it could be good to teach the LegacySpecProvider gatherer to respect the dispatch policy?)

Does that sound reasonable approach? Any memories from the LegacySpecProvider transition that would be relevant?

Alternative, cheekier approach could be to just permit hook_civicrm_entityTypes in the upgrade dispatch policy, but that seems to undermine the point somewhat.

ufundo · 2024-10-04T08:27:24Z

(I also think it could be good to teach the LegacySpecProvider gatherer to respect the dispatch policy?)

In the last commit here I mpved the SearchSegment spec thing from legacy to new model, in order to be more respectful. But that's treating the symptom (loading that was throwing exception during one of the upgrader steps) rather than the cause.

ufundo · 2024-10-04T08:31:54Z

(Aside: with new approach I'm seeing some issues in doCoreFinish that even after the dispatch policy is loosened to upgrade.final, the ClassScanner doesn't seem to pick up extension classes. I think because of its dedicated cache. I tried adding a ClassScanner::cache('index')->clear() call, but it seemed to be ignored. It was late last night so I will investigate more this morn, but if you have any nuggets of wisdom as to why that wouldn't work, might save me some digging.)

ufundo · 2024-10-05T11:34:33Z

A bit of scrabbling around in ConformanceTest at the end there - but I think this should be passing / working now.

ufundo · 2024-10-05T11:39:48Z

Big picture - I think it could be really helpful for this stuff if ClassScanner::get could distinguish between core classes and extension classes from hook_civicrm_scanClasses in what it returns. As it is at the moment there's likely to be a bit of overlap between the ClassScanner entities and ones from LegacyEntityScanner - could be neater and more efficient.

ufundo · 2024-10-05T11:40:35Z

(Expecting an outstanding upgrade fail for standalone php74m56 because of civicrm/civicrm-upgrade-test#14)

totten · 2024-10-07T21:04:07Z

Civi/Api4/Generic/EntityInterface.php

+ * This is currently a null interface - it allows picking
+ * up AbstractEntities via the ClassScanner
+ */
+interface EntityInterface {


Oh, yeah, moving toward a straight-up interface would be a silver-lining. (Tho obviously not the main goal.)

ufundo · 2024-10-08T08:27:23Z

(Expecting an outstanding upgrade fail for standalone php74m56 because of civicrm/civicrm-upgrade-test#14)

Test updated and passing here now 🎉 (vs still failing on master)

ufundo · 2024-10-08T08:28:13Z

All the extended test failures look pre-existing to me (tokens and group management)

ufundo · 2024-10-08T13:55:20Z

Just noting here I can reproduce the crash on D10 on 5.79RC so its potentially a broader regression

colemanw · 2024-10-09T12:04:51Z

@ufundo I just tried changing the base of this PR to 5.79, which I think is correct since this is an unreleased regression, but it looks like you've recently (perhaps accidentally) rebased it agains master so it's picking up extra commits that shouldn't be there. If you could rebase out those extra commits & maybe squash a few of yours like "fix typo" then this ought to be merge-ready.

… provide

… classes

ufundo · 2024-10-09T14:23:00Z

@colemanw - I've rebased, and tried to take out a few commits that make sense in the long run but aren't strictly essential.

Hopefully that hasn't broken any tests, but worth seeing.

Will put up separate PRs against master for the cleanup-y bits.

colemanw · 2024-10-09T16:54:37Z

@ufundo to clarify what this PR does, is the civi.api4.entityTypes hook now being used to load extension entities, or are we not there yet?

ufundo · 2024-10-09T17:41:37Z

@ufundo to clarify what this PR does, is the civi.api4.entityTypes hook now being used to load extension entities, or are we not there yet?

Not there yet. With this change they come through scanClasses . At the moment the ClassScanner doesn't distinguish core and extension classes once it's got them, so they end up bundled together at that point. I think it could be nice to find a way to separate them out and feed the extension ones through the entityTypes specific hook. But needs a bit more thinking.

ufundo · 2024-10-09T22:41:50Z

Passing as before (minus preexisting fails on tokens / groups in the extended)

Not sure how people approach RC testing, but would think good to merge ASAP to catch as much of remaining RC testing period as possible.

ufundo · 2024-10-09T22:42:53Z

ping @totten in case any final thoughts?

eileenmcnaughton · 2024-10-17T02:17:41Z

I just tried the rc with this PR merged & our custom extension no longer installs - I have logged as a regression at https://lab.civicrm.org/dev/core/-/issues/5533

Change-Id: Ic187afdf33be725200d5c51ba90eed66b89b09e2

…undraising/crm into deployment + aeec552 Might be a more flexible place to define the setting + 598d9cc PHpoffice update (partial of civi security update) + d72d016 Security update -minus changes from civicrm/civicrm-core#31198 Change-Id: Ib96e8267b3e98f027debcdbec7efd26e756a8368

civibot bot added the master label Sep 27, 2024

ufundo commented Sep 27, 2024

View reviewed changes

ufundo added the run-standalone Civibot should setup demos+tests for Standalone label Sep 27, 2024

ufundo changed the title ~~Api4 entitymeta use classscanner~~ dev/core#5493 use ClassScanner to find Api4 entities in ActionObjectProvider Sep 27, 2024

colemanw reviewed Sep 27, 2024

View reviewed changes

Civi/Api4/Generic/AbstractEntityInterface.php Outdated Show resolved Hide resolved

colemanw reviewed Sep 27, 2024

View reviewed changes

ufundo force-pushed the api4-entitymeta-use-classscanner branch from 2d2099d to 8901921 Compare September 30, 2024 09:52

ufundo force-pushed the api4-entitymeta-use-classscanner branch 2 times, most recently from 28ca4d2 to 17404b7 Compare September 30, 2024 11:44

ufundo force-pushed the api4-entitymeta-use-classscanner branch from 17404b7 to 8244361 Compare October 1, 2024 11:28

colemanw reviewed Oct 1, 2024

View reviewed changes

ufundo added the sig:unreleased-regression label Oct 3, 2024

ufundo marked this pull request as ready for review October 3, 2024 23:11

ufundo force-pushed the api4-entitymeta-use-classscanner branch 3 times, most recently from ee3a72c to 798138c Compare October 5, 2024 11:33

ufundo added the run-extended-tests Civibot should run comprehensive test suites with versions of UF/PHP/MySQL label Oct 5, 2024

totten reviewed Oct 7, 2024

View reviewed changes

colemanw changed the base branch from master to 5.79 October 9, 2024 12:02

civibot bot added 5.79 and removed master labels Oct 9, 2024

ufundo force-pushed the api4-entitymeta-use-classscanner branch from 798138c to 5a61c11 Compare October 9, 2024 14:11

ufundo added 11 commits October 9, 2024 15:19

Api4 ActionObjectProvider - use ClassScanner interface to get entities

e390909

Api4 ActionObjectProvider - check we know the entity we are trying to…

dc6140e

… provide

CRM_Core_Permission_Standalone - better api availability check

1fd7299

LegacySpecScanner - respect the scanClasses dispatch policy

bb54b42

add LegacyEntityScanner to find entities from extensions without scan…

9e685ba

… classes

ConformanceTest - retain low-tech scanning implementation

9308e58

ConformanceTest - fetch Mock api4 entities in getEntitiesLotech

183dead

ConformanceTest - we need ClassScanner to pick up mock api4 entities

74e6d68

ConformanceTest - add since meta to Api4 mock entities

f61bc2e

ConformanceTest- move MockEntityDataStorage class to fix classloading

77bb4a2

ConformanceTest - limit updateFailsFromCreate to DAOEntities for now

2f880fd

ufundo force-pushed the api4-entitymeta-use-classscanner branch from 5a61c11 to 2f880fd Compare October 9, 2024 14:19

ufundo mentioned this pull request Oct 9, 2024

OAuth - opt into scan-classes to pick up Api4 entities #31254

Merged

colemanw added the merge on pass label Oct 9, 2024

colemanw merged commit 5d6d173 into civicrm:5.79 Oct 14, 2024
2 of 3 checks passed

ufundo mentioned this pull request Oct 14, 2024

5.79 #31294

Merged

wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request Oct 17, 2024

Security update -minus changes from civicrm/civicrm-core#31198

d72d016

Change-Id: Ic187afdf33be725200d5c51ba90eed66b89b09e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#5493 use ClassScanner to find Api4 entities in ActionObjectProvider #31198

dev/core#5493 use ClassScanner to find Api4 entities in ActionObjectProvider #31198

ufundo commented Sep 27, 2024 •

edited

Loading

civibot bot commented Sep 27, 2024

ufundo Sep 27, 2024

colemanw Sep 27, 2024

ufundo Oct 3, 2024

ufundo commented Sep 27, 2024

colemanw Sep 27, 2024

ufundo commented Sep 30, 2024

ufundo commented Sep 30, 2024 •

edited

Loading

colemanw Oct 1, 2024

ufundo commented Oct 4, 2024

ufundo commented Oct 4, 2024

ufundo commented Oct 4, 2024

ufundo commented Oct 5, 2024

ufundo commented Oct 5, 2024

ufundo commented Oct 5, 2024

totten Oct 7, 2024

ufundo commented Oct 8, 2024 •

edited

Loading

ufundo commented Oct 8, 2024

ufundo commented Oct 8, 2024

colemanw commented Oct 9, 2024

ufundo commented Oct 9, 2024

colemanw commented Oct 9, 2024

ufundo commented Oct 9, 2024

ufundo commented Oct 9, 2024

ufundo commented Oct 9, 2024

eileenmcnaughton commented Oct 17, 2024

dev/core#5493 use ClassScanner to find Api4 entities in ActionObjectProvider #31198

dev/core#5493 use ClassScanner to find Api4 entities in ActionObjectProvider #31198

Conversation

ufundo commented Sep 27, 2024 • edited Loading

Overview

Before

After

civibot bot commented Sep 27, 2024

ufundo Sep 27, 2024

Choose a reason for hiding this comment

colemanw Sep 27, 2024

Choose a reason for hiding this comment

ufundo Oct 3, 2024

Choose a reason for hiding this comment

ufundo commented Sep 27, 2024

colemanw Sep 27, 2024

Choose a reason for hiding this comment

ufundo commented Sep 30, 2024

ufundo commented Sep 30, 2024 • edited Loading

colemanw Oct 1, 2024

Choose a reason for hiding this comment

ufundo commented Oct 4, 2024

ufundo commented Oct 4, 2024

ufundo commented Oct 4, 2024

ufundo commented Oct 5, 2024

ufundo commented Oct 5, 2024

ufundo commented Oct 5, 2024

totten Oct 7, 2024

Choose a reason for hiding this comment

ufundo commented Oct 8, 2024 • edited Loading

ufundo commented Oct 8, 2024

ufundo commented Oct 8, 2024

colemanw commented Oct 9, 2024

ufundo commented Oct 9, 2024

colemanw commented Oct 9, 2024

ufundo commented Oct 9, 2024

ufundo commented Oct 9, 2024

ufundo commented Oct 9, 2024

eileenmcnaughton commented Oct 17, 2024

ufundo commented Sep 27, 2024 •

edited

Loading

ufundo commented Sep 30, 2024 •

edited

Loading

ufundo commented Oct 8, 2024 •

edited

Loading