Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Currently, entity API exportables defined in code are incorporated by the controller by getting the defaults from code whenever entities are loaded. However, there are some drawbacks with that workflow:
* The difference between db-centric insert()/update()/delete()/load() hooks for entities in code might be confusing, e.g. when an entity in code is updated, thus overridden hook_insert() is invoked - as it is inserted in the database.
#978832: enable modules to apply configuration (exportable entities) introduces hook_entity_enabled/disabled to enable modules to detect new exportables, however with all entities in the db that won't be necessary at all.
* Applying conditions to entities in code has to be emulated in PHP, what doubles the functionality databases provide.
* As of now there is no EntityFieldQuery support for entities in code, thus an EntityFieldQuery would only return overridden/custom entities.
Disadvantages of storing all entities in code in the db:
* One could not just use t('label') for translated labels any more.
* Updates to entities in code won't be applied automatically any more. Maybe, just re-saving defaults on cache-clear like features suffices?
opinions?
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | entity_exportables.patch | 36.02 KB | fago |
| #7 | profile2_exportable_update.patch | 1.08 KB | fago |
| #1 | entity_exportables.patch | 29.55 KB | fago |
Comments
Comment #1
fagohere is a first patch for the described approach. It's working, but the documentation needs to be fixed and an upgrade path for modules implementing exportable entities is still missing, as those now need to include the entity_exportable_schema_fields() in their base table.
Thus the patch
* fixes #978832: enable modules to apply configuration (exportable entities) and makes it possible to just use regular insert(), update() & delete() for configuration! Finally green tests again.
* fixes the usage of EntityFieldQuery with exportable entities.
* allows usage of ususal queries e.g. for paging in the UI
I also ran into a problem with calling variable_set() during a transaction. See #1007830-9: Nested transactions throw exceptions when they got out of scope. For now I've fixed that by moving that out of the transaction.
>* One could not just use t('label') for translated labels any more.
As entities are translated with queries using the 'translatable' tag anyway, I guess we can use the same approach to "imported" default-entities .
Comment #2
drunken monkeyAs I already stated at least once, I'd find that approach much better. And if these two really are the only disadvantages worth mentioning, then we should do it right away, in my opinion. Re-saving defaults on cache clear, providing some UI for it or just letting users deal with those problems (e.g. by re-enabling the feature modules) are, in my opinion, all viable options for problem #2. And drawback #1 I'm sure we can cope with.
Since translatability would have been gone for overridden entities, doing translation that way would seem problematic to me anyways.
And as one of those immediately affected I can say that always having to think of the default entities when coding is rather annoying, and even makes a few things (OK, the sortable server overview wasn't that great, but there are surely better examples) almost impossible. Having all entities in the database, with no need to care about how they got there, would be a huge DX benefit, in my opinion.
It would also make it possible to let all entities be exportable without any need for a special declaration or treatment, wouldn't it? If I'm not missing something, this would e.g. enable using pre-set 403 and 404 nodes as features, among other things.
Comment #3
drunken monkeySorry.
Comment #4
sunWould it make sense to gather some feedback from friends at devseed for this issue?
Comment #5
drunken monkeyTook a look at the patch (don't think I can really test it, since it would require some changes to my modules, too) and discovered the
entity_exportable_schema_fields()function. (I know, you already mentioned that, but I didn't really think about that.) In my opinion, this is another drawback, although probably still the best option if you want to retain the status information and correctly delete default entities when feature modules are disabled.What other changes would be needed for modules offering (exportable) entities, besides this and the removed and changed hooks?
Comment #6
fagoThe only change for modules is that they have to add in entity_exportable_schema_fields() fields to their schema + add an updated for that. With need those db columns to store the exportable status + the providing module, what as affect also allows to query for that. This is also a nice improvement, as previously that wasn't possible and would require some more magic in the controller.
So we could do the updated automatically via hook_schema_alter(), but I don't think it's a good idea to mess with module's schema. Then currently the API dies if the fields are missing - I guess we should improve that.
Comment #7
fagook, I've updated and completed the patch. Changes:
* fixed docs + some bugs in the previous patch
* added custom error messages if modules need to add-in the schema fields:
Attached is also a patch updating profile2, this should show the changes necessary for any module providing an exportable entity. I also tried to make use of a versioned dependency for profile2, but failed. See #1013302: Versioned dependencies fail with dev versions and git clones.
Comment #8
fagoThe approach taken seems to work fine, so I've committed it.
Summary of changes:
* Defaults provided for exportable entities are now saved to the database too, for that there is rebuild operation automatically triggered upon module installation / update / cache clear. See
entity_defaults_rebuild().* For that to work, modules need to add
entity_exportable_schema_fields()to their base table schema for exportable entities.* Existing modules making use of exportable entities need to provide an upgrade routine to fix the db schema. Look at the profile2 patch for a working example.
* The recently by #978832: enable modules to apply configuration (exportable entities) introduced hooks entity_enabled() and entity_disabled() are deprecated, as with this approach one can just use the regular insert(), update() & delete() hooks for configuration items (-> exportable entities) too!
Thus, this patch comes with this advantages:
* fixes the usage of EntityFieldQuery with exportable entities. However note that the EntityFieldQuery will always return the numeric ids.
* allows making use of db queries, e.g. for paging, filtering and sorting in the UI.
* #978832: enable modules to apply configuration (exportable entities) got fixed with an improved DX.
Comment #9
scor CreditAttribution: scor commentedI was about to reopen this issue as I could not find this patch committed on github, but it seems to have been committed to CVS only. What did I miss? I thought this module, like profile2, was primarily maintained on github?
Comment #10
fagooh, sry the github repo is not up2date anymore, as the official mirror is working fine now I've disabled the syncing script. So best just make use of the official Git mirror.
Comment #11
scor CreditAttribution: scor commentedok, just to make sure nobody else uses the old github repo, why don't you add a note there, or even better, remove the files from that repo, with a note... thanks