With the search API we ran into a problem we have currently generally for exportables: There is no way for modules to apply exported configuration, as e.g. hook_insert() is not invoked for configuration that comes in a default hook.

Proposed solution:
* Leave hook_insert(), hook_update() and hook_delete() alone as they are now. They apply are storage-centric, thus modules extending configuration items may use them to care for their extended-storage.
* Create two new hooks, hook_entity_enable() and hook_entity_disable() - which are invoked independent of the storage and used for exportable entities only. hook_entity_enable() needs to run when a module providing the entity in code is enabled *or* a new item is inserted in the database. Analogously disable needs to run when a providing module is disabled or an item is deleted from the database.
* In case an item gets reverted, disable gets invoked. Then we need to invoke 'enable' again, to enable the item in code.
* We don't care about configuration updates. We cannot track configuration updates for configuration provided in code anyway. If this is relevant, disable + enable is the way to go.
* Having a (generic?) way to enable/disable configuration without having to actually *edit* it would make much sense too.

CommentFileSizeAuthor
#3 entity_activation.patch15.92 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Implementation thoughts:

Store status in the entity status + use variable_get/variable_set() to store it. For that add another bit to the bitmask, e.g. ENTITY_DISABLED - then store the status in:
$conf[$entity_type][$name] = $entity_status;
On module enable, initialize the activation status. If it is not set yet + enabled, a new config is enabled, thus we invoke hook_entity_enable(). On module_disable, we remove the activation status for the configs.

Thus then we can use the flag to also improve checking for overridden entities as follows:
* If an entity is overridden, it is stored in the flag. Thus entity_load() can make use of that information and save a query.
* If a module with entities in code is enabled and an overridden entity already exists in the db, the flag needs to be updated to reflect the entity is now overridden.
* If a module with entities in code is disabled, the flags of overridden entities have to remain but need to be updated to reflect it's now custom.

fago’s picture

related issue discussing exports in JSON: #975758: Entity Import/Export UI

fago’s picture

FileSize
15.92 KB

ok, here is a first patch without the support of an activation flag - thus each existing entity is treated as "activated".

fago’s picture

Status: Active » Needs review

>* In case an item gets reverted, disable gets invoked. Then we need to invoke 'enable' again, to enable the item in code.

I've not implemented it that way, as it doesn't work that way when an entity is overridden either. Both situations are basically configuration updates, which we don't support.

fago’s picture

Status: Needs review » Needs work

The last submitted patch, entity_activation.patch, failed testing.

fago’s picture

I've gone ahead and committed #3 as it's working fine except for the core-issue. Leaving the issue open though for the remaining tasks:
* #979730: fix hook_module_disabled() comment and add tests
* Allow modules to implement an additional "active" key for entities. However that depends on a way to determine when an entity is actually updated to be not active any more. See #651240: Allow modules to react to changes to an entity.

drunken monkey’s picture

As core patches can take a while, as we know: How about using my simpler approach for now (i.e., just checking manually whether the module implements default hooks — maybe extended with alter hooks or even minding hook_module_implements_alter())?
This would at least avoid having broken hooks in your module.

fago’s picture

I prefer fixing bugs instead working around of them. But yes, having this still breaking the tests is very annoying.

@workaround:
Still we'd need to load the entities somehow, and without using entity_load() we'd have to duplicated that code. However, as noted in the linked issue, module_invoke() still works. So we'd could work around it by re-implementing module_invoke_all() with single module_hook() + module_invoke() calls - :D

fago’s picture

Priority: Normal » Major
drunken monkey’s picture

Doesn't the hook already return loaded entities? The duplicated would only need to call the alter hooks, as far as I can see. There also isn't any need to load the other entities, if I'm not mistaken.

I tested the core patch, but it failed for me — either the patch or your code in hook_modules_diabled() seems to have a bug. At least for me, hook_search_api_index_disabled() wasn't called when disabling a module defining a default index. hook_search_api_index_enabled() was called, on the other hand — with or without the patch — so that part works.

PS: OK, tested a bit further and the core patch seems to be the issue — see there.

Of course, fixing bugs is a lot better then doing workarounds, but even when the core patch is RTBC, it could take weeks or even months until it is committed — and once Drupal 7 is released, one shouldn't rely on patches in HEAD, anyways. So a final release would then have to wait at least for Drupal 7.1.
Having a working solution (even if it's just minimum code with some problems in edge cases) would help a lot, I think.

fago’s picture

>Doesn't the hook already return loaded entities? The duplicated would only need to call the alter hooks, as far as I can see. There also isn't any need to load the other entities, if I'm not mistaken.

Yes, but it's not simple. Entities might be overridden, and entities defined in code get some properties set by default. This all has to be obeyed, which would basically mean duplicating entity_load()...

>I tested the core patch, but it failed for me — either the patch or your code in hook_modules_diabled() seems to have a bug.
Hm, the entity comes with a test case for that too. Does that work when you have the core patch applied? For me it does, what means the entity_type specific hook is invoked.

fago’s picture

>PS: OK, tested a bit further and the core patch seems to be the issue — see there.
You have not written anything about any specific issue, so what issues do you have?

drunken monkey’s picture

Sorry, my bad — made a stupid mistake when testing. Everything works fine for me, after applying the core patch.

fago’s picture

see #1008810: Store all exportables in the db - this would also solve this issue without the need for any additional hooks.

fago’s picture

Component: Entity CRUD API - main » Core integration
Status: Needs work » Fixed

fixed with #1008810: Store all exportables in the db. Thus hook_*_enabled|disabled() is deprecated by just using the regular hooks insert(), update() & delete() in order to detect new/updated/deleted exportables!

quazardous’s picture

Status: Fixed » Active

function entity_exportable_schema_fields($module_col = 'module', $status_col = 'status') could be in .module so it can used by other modules.

and if the functionality evolves it s good that we have to use a dedicated function.

fago’s picture

Status: Active » Fixed

Yep, it's intended to be re-used so I've fixed that already. I thought drupal always includes all install files if it does, but it turned out it doesn't do that during uninstalling.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.