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.
Comment | File | Size | Author |
---|---|---|---|
#3 | entity_activation.patch | 15.92 KB | fago |
Comments
Comment #1
fagoImplementation 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.
Comment #2
fagorelated issue discussing exports in JSON: #975758: Entity Import/Export UI
Comment #3
fagook, here is a first patch without the support of an activation flag - thus each existing entity is treated as "activated".
Comment #4
fago>* 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.
Comment #5
fagorequired core patch: #979730: fix hook_module_disabled() comment and add tests
Comment #7
fagoI'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.
Comment #8
drunken monkeyAs 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.
Comment #9
fagoI 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
Comment #10
fagoComment #11
drunken monkeyDoesn'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.
Comment #12
fago>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.
Comment #13
fago>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?
Comment #14
drunken monkeySorry, my bad — made a stupid mistake when testing. Everything works fine for me, after applying the core patch.
Comment #15
fagosee #1008810: Store all exportables in the db - this would also solve this issue without the need for any additional hooks.
Comment #16
fagofixed 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!
Comment #17
quazardous CreditAttribution: quazardous commentedfunction 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.
Comment #18
fagoYep, 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.