Move dynamic parts (view modes, bundles) out of entity_info()

Could we agree to move 'list of view modes' and 'list of bundles' out of entity_info and into separate methods on the Entity class ?

Would 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 ?

We 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()

Moving 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 :-)

Of 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 :-)

"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...

#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.

I guess #1552396: Convert vocabularies into configuration applies to this too.

For 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?

@sun - need to read that one. Is the goal to make that the *only* way to define bundles ?

could 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)

I'd strongly support @ tim.plunkett's idea in #8

I would much rather introduce hook_entity_view_modes_info and hook_entity_bundles_info.

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.

It 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 ;-) )

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.

+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)

Yay. 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.

Doh, right. I shouldn't review patches from my mobile.

#29: bundles-view-modes-1822458-29.patch queued for re-testing.

This 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.

+++ b/core/includes/entity.inc
@@ -49,23 +49,84 @@ function entity_get_info($entity_type = NULL) {
+  $bundles = &drupal_static(__FUNCTION__, array());

Are 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.

+++ b/core/includes/entity.inc
@@ -49,23 +49,84 @@ function entity_get_info($entity_type = NULL) {
+    // If no bundles are provided, use the entity type name and label.
+    if ($fallbacks) {
+      foreach (entity_get_info() as $type => $entity_info) {
+        if (!isset($bundles[$type])) {
+          $bundles[$type][$type]['label'] = $entity_info['label'];
+        }
+      }

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

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
index 7ba8913..0000000
--- a/core/lib/Drupal/Core/Plugin/Discovery/InfoHookDecorator.php
+++ /dev/null

YAY!!

+++ b/core/modules/rdf/lib/Drupal/rdf/SiteSchema/SiteSchemaManager.php
@@ -38,14 +38,15 @@ public function writeCache() {
-    foreach (entity_get_info() as $entity_type => $entity_info) {
+    foreach (entity_get_bundles() as $entity_type => $bundles) {
       // Only content entities are supported currently.
       // @todo Consider supporting config entities.
+      $entity_info = entity_get_info($entity_type);
       $reflection = new ReflectionClass($entity_info['class']);

We could save some function calls by moving entity_get_info() above the foreach.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyStorageController.php
@@ -100,7 +100,8 @@ protected function postDelete($entities) {
-    cache_invalidate_tags(array('content' => TRUE, 'entity_info' => TRUE));
-    drupal_static_reset('entity_get_info');
+    cache_invalidate_tags(array('content' => TRUE));
+    entity_info_cache_clear();

If we add db caching, this should be reverted.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -106,12 +106,29 @@ function taxonomy_permission() {
+  $info['taxonomy_vocabulary']['full'] = array(
+    'label' => t('Taxonomy vocabulary'),
+    'custom_settings' => FALSE,

Erm.. wat? Config entities have no view modes afaik :)

Removing the Configuration system tag because this has nothing to do with it.

re tagging.
Edited: Sorry for re tagging.

why? i just said that tag is not necessary for this issue.

+++ b/core/includes/entity.api.php
@@ -11,24 +11,110 @@
+function hook_entity_view_modes_info() {

Hm. Typically, our info hooks are singular (not plural); i.e., hook_entity_view_mode_info()

+++ b/core/includes/entity.api.php
@@ -11,24 +11,110 @@
+function hook_entity_bundles_info() {
+  $info = array();
...
+function hook_entity_bundles_info_alter(&$bundles) {
+  $bundles['user']['user']['label'] = t('Full account');

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

+++ b/core/includes/entity.api.php
@@ -11,24 +11,110 @@
+  return $info;

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 :)

+++ b/core/includes/entity.inc
@@ -49,23 +49,91 @@ function entity_get_info($entity_type = NULL) {
 function entity_info_cache_clear() {
...
+  drupal_static_reset('entity_get_view_modes');
+  drupal_static_reset('entity_get_bundles');
...
+function entity_get_bundles($entity_type = NULL) {
...
+function entity_get_view_modes($entity_type = NULL) {

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

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -239,7 +193,6 @@ class EntityManager extends PluginManagerBase {
-    $this->discovery = new InfoHookDecorator($this->discovery, 'entity_info');

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?

+++ /dev/null
@@ -1,71 +0,0 @@
-class InfoHookDecorator implements DiscoveryInterface {

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.

+++ b/core/modules/file/file.module
@@ -90,6 +90,18 @@ function file_element_info() {
+function file_entity_view_modes_info() {
...
+    'custom_settings' => FALSE,

Can't we default that to FALSE?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyStorageController.php
@@ -100,7 +100,8 @@ protected function postDelete($entities) {
   public function resetCache(array $ids = NULL) {
...
-    cache_invalidate_tags(array('content' => TRUE, 'entity_info' => TRUE));
-    drupal_static_reset('entity_get_info');
+    cache_invalidate_tags(array('content' => TRUE));
+    entity_info_cache_clear();

Hm. I think it would make sense to keep the entity_info cache tag for all three caches (combined).

If we decide to keep the [InfoHookDecorator] 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.

Can'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.

Because we have no use case.

ET 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.

What @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()."

To add any information to any entity types not provided by your module, you do and still use the alter hook.

Again, 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.

I am generally convinced by this argument, and also want to see this move forward. This looks like a sane fix to me.

Eclipse

That's not correct. Info hooks translate into module_invoke_all(), which has exactly this architectural design built-in, for many many years already.

I 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.

@tim.plunkett:

@plach, actually, that uses hook_entity_info_alter(), and this patch doesn't change that.

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

We're not removing the alter hook, just the info hook that duplicates the annotation.

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

In D7 ET does exactly this: http://drupalcode.org/project/entity_translation.git/blob/refs/heads/7.x...

@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.

That 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.

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?

The 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 :-)

That's not correct. Info hooks translate into module_invoke_all(), which has exactly this architectural design built-in, for many many years already.

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.

Looks good to me and seems to have quite a bit of support. Committed to 8.x. Thanks.

This needs a change notice.

Looks 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.

Followup to remove a stray @todo: #2008558: Move nonalterations from hook_entity_bundle_info_alter() implementations to hook_entity_bundle_info() implementations.

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.