hook_managed - do not try to disable managed entities if is_active is not available to api3.create

[eileenmcnaughton]

Overview

Only try to disable disable-able entities from managed
Respect cleanup = never on disableEntities

Before

 // FIXME: if ($dao->entity_type supports is_active) {
    if (TRUE) {

Currently cleanup=never is respected on ininstall but not on disable

After

Check is done to ensure the entity can be disabled (using v3 api)

Technical Details

I'm hitting this as a bug on a cleanup=never entity which has a v4 api and not a v3 api so it could be
solved using more failure tolerance in the disable function or checking more values/ entity availability

We discussed adding the 'activate' key and while I think that is a good idea I hit some implementation issues - I couldn't see how to do that without adding a field to civicrm_managed since the information is otherwise unavailable at the right time.

However, I did note that I could resolve a TODO in the code and at the same time ameliorate the issue I was dealing with

However, I actually think the intuitive meaning of cleanup = never is 'never try to clean this up
' and I don't see why that would apply to uninstall & not disable

Comments

@totten @colemanw

[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).

[seamuslee001]

@eileenmcnaughton test fail relates here

CRM_Core_ManagedEntitiesTest::testDeactivateReactivateModule
Failed asserting that '1' matches expected 0.

/home/jenkins/bknix-dfl/build/core-20144-16l73/web/sites/all/modules/civicrm/tests/phpunit/CRM/Core/ManagedEntitiesTest.php:399
/home/jenkins/bknix-dfl/build/core-20144-16l73/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:226
/home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar:615

[eileenmcnaughton]

@seamuslee001 yeah - but I don't know whether to try to fix that or not as @totten said he is commenting & I'm waiting on that

[totten]

OK, so restating context for comprehension/clarity... the general idea of a "managed entity" is for the entity lifecycle to track the extension (ie update, enable, disable, remove in tandem with the owner ext).

The use-case you described (disabling an APIv4-only entity) does sound problematic, though from my POV it raises another issue. Narrowly, reconcileDisabledModules() calls disableEntity(), but disableEntity() doesn't support APIv4. Broadly, CRM_Core_ManagedEntities predates APIv4 and therefore doesn't support it. There are ~7 places where it calls-out to civicrm_api()... and probably all of those need to be trained on v3-vs-v4 differences.

Anywho, it sounds like the approach here is -- "If disableEntity() isn't working, then let's opt-out of enable/disable-flagging." And there are currently two opt-out options:

• update (always, never): When the extension is updated, do you also update the record?
• cleanup (always, unused, never): When the extension is removed, do you also remove the record?

Hypothetically, you could have a flag for the activate/deactivate behavior, eg

• activate (always, never): When the extension is (de)activated, do you also (de)activate the record?

This current PR re-purposes the cleanup policy as an activation policy. I can see how that's good enough for your current v3/v4 work-around-issue... actually, for the v3/v4 issue, one should opt-out of everything that ManagedEntities does because it just doesn't understand v4. But let's set that aside. There are other cases where one might opt-out of using disableEntity(). Would they be better served by a new flag or by interpreting cleanup more broadly?

IIRC, the other case for opting-out of disableEntity() is when you're trying to manage an entity that fundamentally lacks support for (de)activation (ie lacks is_active). But to my mind, that's a different a driver compared to cleanup:

• update: Choose always if you want to exercise control over the content. Choose never if you want to provide a template/starting-point for downstream customization. (Driver(s): How much you defer to downstream users.)
• cleanup: Choose always if you want to strictly follow the "uninstall" semantics. Choose unused if you're paranoid about accidental data-loss. Choose never if you're crazy. (Driver(s): FKs and data-retention.)
• activate: Choose always if the target-entity supports activation (is_active). Choose never if the target-entity does not support activation and if you're willing to accept that the stale entity will remain active during the disabled period. (Driver(s): the data-model of the target entity.)

Suppose, for example, that you're trying to use hook_managed to register a civicrm_grant or civicrm_note. These entities have APIv3 support, so there's really no problem in applying the update and cleanup mechanics. But they lack the field is_active. Consequently, you would want activate=>never, even though update=>always, cleanup=>always, and cleanup=>unused are quite fine.

[eileenmcnaughton]

@totten so activate doesn't exist but you are suggesting it could?

[totten]

@eileenmcnaughton Yup

[eileenmcnaughton]

@totten after digging into adding 'activate' handling I realised it was getting pretty tricky as the mgd data was already gone by the time I needed it (I guess another DB field might be needed).

As a stop gap I have resolved the FIXME in the code

[mattwire]

I think this needs reviewing/looking at in conjunction with #19676

[eileenmcnaughton]

It's the same class but it doesn't directly interact

[eileenmcnaughton]

@totten I guess this PR & @mattwire's PR (#19676) both need tests - but it would be good for you to confirm the approach for both PRs. I found the test class kinda hard going but I'll dig in again if you confirm this is otherwise OK & I'm sure @mattwire will be happy to write a test for his PR if he knows the same

[totten]

@eileenmcnaughton Yeah, I think this approach looks pretty good. You had flagged this for prioritization before 5.38 flips to RC (which is ASAP). If we're aiming for the test now -- that means we push to 5.39? (Or, if you want, we might squeeze this one into 5.38 and then merge UT to 5.38.beta?)

I've added some comments/suggestions on #19676. I think it merits a bit more attention - so harder to get to 5.38. Feels more like 5.39 material.

[eileenmcnaughton]

@totten if you merge this now I'll take a look at a test as follow up

[totten]

OK, I'm giving it an r-run.

[eileenmcnaughton]

@totten easies r-run is with monolog extension https://github.com/eileenmcnaughton/monolog

[totten]

Well, I'd made a couple throw-away extensions to test this. The first created a new entity-type/table (Apple/civicrm_apple). This is a persnickity entity which says:

function civicrm_api3_apple_create($params) {
  if (isset($params['is_active'])) {
    throw new \API_Exception("We should not be called with is_active!");
  }
  return _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $params, 'Apple');
}

The second extension had two managed-entities:

• One entity used entity=>ReportTemplate (ie a standard core entity that supports is_actve)
• The other used entity=>Apple (ie my test/throw-away-example which does not support is_active).

The patch correctly skipped de-activation. However, it still produced re-activation. I've added a couple commits to fix that as well. I tried various workflows of enable/disable/uninstall steps... and they seemed to work as I expected. @eileenmcnaughton, if the updates are OK with you, then let's merge.

[eileenmcnaughton]

Thanks @totten - looks good - I was aware that the scope was pretty narrow so that is great.

[eileenmcnaughton]

I'll try again at those tests

[eileenmcnaughton]

Looks like this missed the forking after all - ah well - we tried

[eileenmcnaughton]

I made good on my promise to add a unit test - #20320 - although in the end this PR could have stayed open waiting for the test as it didn't hit the rc anyway