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.
#1763974: Convert entity type info into plugins removes hook_entity_info(), where all bundles and view modes are added by core modules, by moving those to hook_entity_info_alter().
However, mymodule_entity_info_alter() would see comment and file entity types, but not node, taxonomy term, or user, because of the alphabetical weights.
This might be made moot by #1043198: Convert view modes to ConfigEntity and #1782460: Allow to reference another entity type to define an entity type's bundles.
Comment | File | Size | Author |
---|---|---|---|
#53 | bundles-view-modes-1822458-53.patch | 57.69 KB | effulgentsia |
#53 | interdiff.txt | 4.07 KB | effulgentsia |
#43 | bundles-view-modes-1822458-43.patch | 60.71 KB | tim.plunkett |
#43 | interdiff.txt | 17.32 KB | tim.plunkett |
#41 | bundle-view-mode-1822458-41.patch | 61.01 KB | tim.plunkett |
Comments
Comment #1
yched CreditAttribution: yched commentedCould we agree to move 'list of view modes' and 'list of bundles' out of entity_info and into separate methods on the Entity class ?
Comment #2
tim.plunkettI'm not against it, but how would it work with alters?
Comment #3
yched CreditAttribution: yched commentedWould need new, separate alter hooks, I guess.
I mean, benefiting from hook_entity_info_alter() cannot be the reason to keep stuff in the giant entity_info array while they could / should live elsewhere, right ?
Comment #4
sunWe should fix this directly in #1763974: Convert entity type info into plugins, because it presents a rather large regression for all modules that are extending other entity type modules.
The standard way is to prepend a build operation before the alter operation; i.e.:
hook_entity_info_build()
hook_entity_info_alter()
Comment #5
tim.plunkettThe only two valid entity info keys that need to be handled are view_modes and bundles.
I would much rather introduce hook_entity_view_modes_info and hook_entity_bundles_info.
That would be easy to do and would also not overly complicate the efforts to covert view modes to ConfigEntity.
Comment #6
yched CreditAttribution: yched commentedMoving view modes and bundles, as well as any other dynamic part (if any), out of entity info is likely to become an absolute necessity, now that #1735118: Convert Field API to CMI moves $field and $instance to CMI ConfigEntities, and triggers a series of sweet-frigging-mother-of-Jesus dependency hell issues.
Reading a field definition from config means an entity_load(), meaning the whole damn entity_info data must be built first.
That is, in order to get a field definition, we need to get the list of all bundles on all entity types first. How sweet is that, performance wise :-)
Of course, there are cases where the list of bundles or view modes on an entity type depends on the list if fields in other entity types (field_collection, to give an example, there are others). There you have it, screwed :-)
Comment #7
yched CreditAttribution: yched commentedOf course, moving the dynamic parts out of entity_info doesn't exclude reconsidering whether
"fields, instances, views, image styles, contact categories, breakpoints, etc... " and "nodes, users, terms, commerce items, etc..." being
in the same flat list of "entity types whose metadata we need to collect as a whole before being able to read any configuration at all" is really a fine place to be. But that's for another issue :-)
Comment #8
yched CreditAttribution: yched commented"view modes" being read from ConfigEntities is of course another reason why they need to get out of entity_info (recursion).
#1043198: Convert view modes to ConfigEntity currently hacks around it, but it stays a hack - and would hit contrib even if that one didn't make it in.
Why is this postponed, again ? Temptatively un-postponing...
Comment #9
yched CreditAttribution: yched commented#111715: Convert node/content types into configuration has to use the same hack - reading ConfigEntities through direct config() calls instead of the regular load flow.
Bumping to major, and re-titling.
Comment #10
swentel CreditAttribution: swentel commentedI guess #1552396: Convert vocabularies into configuration applies to this too.
Comment #11
sunFor the bundles definition, I think it would make more sense to do #1782460: Allow to reference another entity type to define an entity type's bundles
Which in turn would leave this issue to focus on view modes only.
Objections?
Comment #12
yched CreditAttribution: yched commented@sun - need to read that one. Is the goal to make that the *only* way to define bundles ?
Comment #13
fmizzell CreditAttribution: fmizzell commentedcould we use a plugin-system-style discovery mechanism (hook, config, other entity types) that way we can get all of the options suggested here, and even more if we make the discovery mechanisms extendable (which is not the case for the plugin system at the moment, other than by adding a new decorator in the code of the manager)
Comment #14
gddComment #15
Jose Reyero CreditAttribution: Jose Reyero commentedI'd strongly support @ tim.plunkett's idea in #8
As I think 'alter' hooks are not intended to add new kinds of information, but to alter existing ones. So using an alter hook for this would be some really bad DX.
However what I think we should do is getting rid of hook_entity_info(), we don't really need it, it's unpractical since it seems we usually need that info before hooks are properly initialized and in every case, AFAIK we have the entity type beforehand.
What about properly namespacing entity types so we know which module defines them? Like 'taxonomy.term'? And this way we know where to find some definition for it, that should be something like 'Drupal/taxonomy/entity/Term' or 'Drupal/taxonomy/Plugin/Core/Entity/Term', or whatever you like, just the same for all.
Comment #16
tim.plunkett@Jose Reyero thanks for the support :)
But hook_entity_info() is already gone, and entity types are already namespaced:
Drupal\taxonomy\Plugin\Core\Entity\Term
http://drupalcode.org/project/drupal.git/blob?f=core/modules/taxonomy/li...
Comment #17
tim.plunkettEclipseGC reminded me that #1763974-203: Convert entity type info into plugins reintroduced hook_entity_info() after I killed it once :) But the point remains, it'd be great to re-kill that and have hook_entity_view_modes_info() and hook_entity_bundles_info()
Comment #18
Jose Reyero CreditAttribution: Jose Reyero commentedIt seems I am always a few patches behind :-)
What I mean, updating my language, is we don't need entity types to be "discoverable" (that may be the word, not sure) so it doesn't really matter if we have the hook or not. Or in different words, we don't need to prefetch any list of entity types. We just need to know where to get the entity from when a module uses it.
About namespaces, yes the classes are namespaced, of course, but not the entity types (which I think is 'taxonomy_term', isn't it?). I mean it should be 'taxonomy.term' (module/object_type) and it should be referred always by this. And this is what is preventing us to map exactly the class from the entity type. And this why we need all the discovery overhead.
Anyway, I just wanted to float the idea, but I think we better focus on the original issue for which I think creating these additional 2 hooks would be great.
(And btw I still don't loose the hope to kill annotations at some point ;-) )
Comment #19
sun+1 This has been discussed in the past already, but never as a dedicated issue, so I finally created one:
#1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term)
Comment #20
tim.plunkettWorking on this.
Comment #21
tim.plunkettI broke something to do with rdf and translation, but everything else seems to be working.
Comment #23
tim.plunkettThis should fix the translation tests.
Comment #25
tim.plunkettAnd that fixes the RDF tests!
Comment #26
yched CreditAttribution: yched commentedYay. Thanks a lot for kicking this !
Only remark after an initial (mobile) review : entity_get_bundles($entity_type) will raise a notice if the entity type is unknown (or defines no bundles ?). I think the common practice would be to return an empty array in this case. Same for view modes.
Comment #27
tim.plunkettThat's exactly what we do for unknown entity types, in both functions.
For bundles, we preserve the same fallback behavior, where the entity type name is used (think user entity/bundle pair)
---
In addition to the 4 new hooks and the new entity_get_view_modes(), there are 3 API changes:
array_keys(entity_get_bundles($entity_type));
Comment #28
yched CreditAttribution: yched commentedDoh, right. I shouldn't review patches from my mobile.
Comment #29
tim.plunkettRerolled for blocks as plugins
Comment #30
Berdir#29: bundles-view-modes-1822458-29.patch queued for re-testing.
Comment #31
BerdirThis looks great to me conceptually.
We need to add caching to it however. node.module (node type reset) and rdf.module (loads of queries) make entity_get_bundles() a very costly operation and seems to be called ~9 times per rendered node.
Comment #32
BerdirComment #33
tim.plunkettThe interdiff ignores whitespace, since its 90% indenting.
I figure entity_info_cache_clear() is still a reasonable place to put those.
Comment #35
tim.plunkettAdjusted that drupal_static check
Comment #37
tim.plunkettArgh, combined #1885644: ImageStyle::uri() is broken and has no coverage into here by mistake
Comment #38
amateescu CreditAttribution: amateescu commentedAre we sure static caching is enough? My impression is that we should also cache this in the database and we could use the same 'entity_info' tag so entity_info_cache_clear() stays unchanged.
We should pass $entity_type to entity_get_info(). Also, it would be very nice if we wouldn't have to go through all the entity_get_info() cycle when we just need the type name and label.. something like entity_type_get_names().
YAY!!
We could save some function calls by moving entity_get_info() above the foreach.
If we add db caching, this should be reverted.
Erm.. wat? Config entities have no view modes afaik :)
Removing the Configuration system tag because this has nothing to do with it.
Comment #39
jibranre tagging.Edited: Sorry for re tagging.
Comment #40
amateescu CreditAttribution: amateescu commentedwhy? i just said that tag is not necessary for this issue.
Comment #41
tim.plunkettOkay, fixed to use the same caching.
Because this is used to build the whole bundle info, we need to process all of it. Yes entity_type_get_names() would be nice, but that's all up for altering by hook_entity_info_alter(), so we can't use the partial info.
Moved entity_get_info() out of the foreach
That
drupal_static_reset('entity_get_info');
call was the only one in core outside of entity_info_cache_clear()Just moving code :)
---
I was also able to clean up some weirdness in entity_get_bundles(), yay.
If this passes, we should be done here.
Comment #42
sunHm. Typically, our info hooks are singular (not plural); i.e.,
hook_entity_view_mode_info()
1) Ditto. :)
2) Let's also skip the $info = array() declarations. :)
3) Typically, we use variable names that map to the thingie being returned in info hooks; e.g., $modes, $bundles. Funnily, you did that for the alter hooks, but not for the info hooks :-D
That said, it really feels weird to introduce new info hooks in D8 at this point. ;)
But yeah, I agree that plugins/classes would be 1000% overhead for this.
But still, it almost feels like as if you need to have a really good excuse like this in order to get a patch like this into core these days :)
And that said, I'd actually think it should be possible to turn those API functions into services in a follow-up issue? With the new module handler, we might even be able to automatically re-instantiate them when the active module list changes.
In other words, info hooks are totally fine, but we can try to turn the discovery/collector functions into actual services. :)
Hm. I'm not sure whether it is a wise idea to remove
hook_entity_info()
entirely. Contributed modules do not have the capability of adding new core entity system APIs.The lack of a add/alter hook separation is essentially what made Profile2 and other entity-api-centric modules in contrib a PITA in D7. The only way to make them (and their integration modules) reliably work was to use dirty
hook_module_implements_alter()
implementations for the corresponding info hooks.Wondering what @fago thinks?
Regardless of whether we keep the info hook for entity types, can we keep this decorator in core, please?
It's already done and already exists. There will be use-cases for this in contrib. That's guaranteed.
Can't we default that to FALSE?
Hm. I think it would make sense to keep the entity_info cache tag for all three caches (combined).
Comment #43
tim.plunkettI cleaned up the names of the hooks, and variables used inside them. But 100% agreed, these should just be hooks. If we want them to be anything else, its #1043198: Convert view modes to ConfigEntity or #111715: Convert node/content types into configuration.
Re: services, that'd be a nice follow-up.
The entire reason I worked on this issue was to remove hook_entity_info() and InfoHookDecorator. If we decide to keep the functionality, we need a new name for this pattern. Maybe 'entity_info_add' as you basically referred to it? But it should only be for things that are NEVER in annotations.
Yep, it already has that as a default.
No idea what your last comment means, entity_info_cache_clear() clears the entity_info cache tag, which is used for all three caches.
Comment #44
sunCan't we just keep it as-is? I also don't understand why it shouldn't be allowed when annotations are involved.
Annotations are actually a major part of the problem: With pure info hooks, e.g.,
hook_whatever_info()
,module_invoke_all()
invokes all implementations and - now the important part - merges all results recursively into each other. This allows all modules to add to all other modules, by design. — With annotations, that's not possible, since you'd have to create a "stub" class with an incomplete annotation that isn't supposed to form its own thing/plugin, but merely exists to extend another definition, and which of course, would only be possible if the processing of definitions would recursively merge all discovered plugin data (which I don't think is the case).Therefore, I'd suggest to simply not remove
hook_entity_info()
and InfoHookDecorator from this patch, and simply call this done.Comment #45
tim.plunkettWhat would you propose be left as the documentation for why to use hook_entity_info()? Because we have no use case.
InfoHookDecorator was originally called InfoHackDecorator for a reason. It's a hack, and removing it is the entire premise of this issue.
Comment #46
plachET is adding a host of entity info keys (which atm are documented in the Entity Manager). Hopefully many of them will be revisited/refactored to be more generic, but some of them will remain. In D7 ET is able to work just because it is able to augment entity information to implement a functionality dealing with entities in general. IMO leaving the possibility to add entiy info keys is crucial.
Comment #47
sunWhat @plach said. It's important that something like ET and similar modules are adding to the info, not altering. After adding, contributed and custom modules still need to be able to alter.
Regarding the API docs, this isn't the first hook that isn't implemented in core, but that's just a fundamental design aspect of hooks in the first place. Looking at the original/removed phpDoc, that appears to be just right?
If anything, I'd merely add a clarification:
"Only use this hook to add information to entity types. To change information after all modules have added their data, use hook_entity_info_alter()."
Comment #48
tim.plunkett@plach, actually, that uses hook_entity_info_alter(), and this patch doesn't change that.
We're not removing the alter hook, just the info hook that duplicates the annotation.
An info hook should only be touching entity types provided BY the module, which is what is in HEAD, and is no longer needed in core. To add any information to any entity types not provided by your module, you do and still use the alter hook.
Comment #49
sunAgain, that's a fundamental design flaw. It forces all contrib into using
hook_module_implements_alter()
, since contrib extends contrib, a.k.a. multidimensional modularity.The architectural design rule to prevent that is to clearly separate add vs. alter. That guarantees that all alter operations happen in the designated step, and no module (ab)uses the alter step to add additional data. The alter hook of one module is invoked before the alter hook of other modules.
This really isn't hard. First collect and add, then allow to alter. Dead simple.
Comment #50
tim.plunkettWe don't have this pattern ANYWHERE else. We introduced it here as a hack, and are now cleaning it up.
In D7 you could not use hook_entity_info() to provide data on behalf of any other entity types. Nor can you do that sort of thing in any other info hook. You own data in your info hook.
Even if we did add this "add" step, it doesn't correlate to info hooks at all, since they are all about the collection.
But all it comes down to: we had this patten nowhere in any release of Drupal, and we shouldn't add it here with no use case.
The one described by plach is and has always been during alter.
Comment #51
EclipseGc CreditAttribution: EclipseGc commentedI am generally convinced by this argument, and also want to see this move forward. This looks like a sane fix to me.
Eclipse
Comment #52
sunThat's not correct. Info hooks translate into module_invoke_all(), which has exactly this architectural design built-in, for many many years already.
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedI haven't reviewed the #43 patch or this issue's comments, but from the little I skimmed here, it does seem to me like removing hook_entity_info() entirely is outside the scope of this issue. Can we punt that to a followup, and proceed with just the part of the issue that doesn't appear controversial? Here's a quick stab at that.
Comment #54
plach@tim.plunkett:
In D8 ET uses the alter hook to provide defaults for any missing key it requires. I believe this is one of the correct usages of
hook_*_info_alter()
.I'm not concerned about ET, luckily it is in core, but were it in contrib it should rely on the alter hook to provide the actual entity info key definitions. And I agree with #49 that this is asking for troubles.
In D7 ET does exactly this: http://drupalcode.org/project/entity_translation.git/blob/refs/heads/7.x...
Comment #55
tim.plunkett#53 defeats the entire purpose of why I bothered to work on this issue, but it's better than an edit war. Thanks @effulgentsia.
Comment #56
plach@tim:
I'd be totally open to leverage another way to augment entity info and kill the info hook, but saying that there is no use case for it is simply false.
Comment #57
Crell CreditAttribution: Crell commentedThat usage of an info hook (the ET link in #54) is totally weird. Most info hooks don't do a sane merge of such data. That hook_entity_info() is even capable of that is unusual, and a surprise to me.
Comment #58
fagoThe problem in D7 are caused by hook_entity_info() being used for multiple things, i.e. the system had to build entity-info, bundle-info and view-modes at once. With separating this into multiple steps (as this patch does) bundle-info can safely rely on entity-info to build. So this patch will help us here :-)
I agree with sun here. While it's not well known, it's documented behaviour that module_invoke_all() merges keys down the tree. I agree with sun that we clearly need to separate add/alter steps for our alter-ing paradigm to work out, but my experiments with adding keys to existing info structures via info hooks resulted in problems: You have to blindly rely on the item you are adding something to to be there - if it vanishes for whatever reason, you are creating a broken info-item. That said, this might justify another pre-alteration add-key step, but I do think this applies generally to info/alter hooks and such should be discussed in its separate issue.
My 2 cents are that we should move on with this clean-up and re-iterate on the altering problem in its own issue + apply the results to all info/alter hooks used.
Comment #59
Dries CreditAttribution: Dries commentedLooks good to me and seems to have quite a bit of support. Committed to 8.x. Thanks.
Comment #60
swentel CreditAttribution: swentel commentedThis needs a change notice.
Comment #61
tim.plunkettAdded change notice here http://drupal.org/node/1901332
Comment #62
tstoecklerLooks good, awesome. I added some additional info (http://drupal.org/node/1901332/revisions/view/2539004/2539056). Going straight back to fixed, though. The critical status is so that we don't forget, which we haven't. :-) We can always improve if someone doesn't like my changes or wants add other stuff.
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedFollowup to remove a stray @todo: #2008558: Move nonalterations from hook_entity_bundle_info_alter() implementations to hook_entity_bundle_info() implementations.
Comment #65
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.