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

Files: 
CommentFileSizeAuthor
#53 bundles-view-modes-1822458-53.patch57.69 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 50,677 pass(es).
[ View ]
#53 interdiff.txt4.07 KBeffulgentsia
#43 bundles-view-modes-1822458-43.patch60.71 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,691 pass(es).
[ View ]
#43 interdiff.txt17.32 KBtim.plunkett
#41 bundle-view-mode-1822458-41.patch61.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,654 pass(es).
[ View ]
#41 interdiff.txt4.31 KBtim.plunkett
#37 bundles-views-modes-1822458-37.patch60.65 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,558 pass(es).
[ View ]
#35 bundles-view-modes-1822458-35.patch62.7 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,393 pass(es), 51 fail(s), and 6 exception(s).
[ View ]
#35 interdiff.txt1.43 KBtim.plunkett
#33 bundles-view-modes-1822458-33.patch59.82 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,606 pass(es), 47 fail(s), and 6 exception(s).
[ View ]
#33 interdiff.txt1.72 KBtim.plunkett
#29 bundles-view-modes-1822458-29.patch59.37 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,510 pass(es).
[ View ]
#25 bundles-view-modes-1822458-25.patch59.37 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,028 pass(es).
[ View ]
#25 interdiff.txt896 bytestim.plunkett
#23 bundles-view-modes-1822458-23.patch57.76 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,035 pass(es), 9 fail(s), and 9 exception(s).
[ View ]
#21 bundles-view-modes-1822458-21.patch55.29 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,971 pass(es), 164 fail(s), and 63 exception(s).
[ View ]

Comments

yched’s picture

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 ?

tim.plunkett’s picture

I'm not against it, but how would it work with alters?

yched’s picture

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 ?

sun’s picture

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

tim.plunkett’s picture

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

yched’s picture

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

yched’s picture

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

yched’s picture

Status:Postponed» Active

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

yched’s picture

Title:Add in dynamic bundles and view modes before hook_entity_info_alter()» Move dynamic parts (view modes, bundles) out of entity_info()
Priority:Normal» Major

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

swentel’s picture

sun’s picture

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?

yched’s picture

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

fmizzell’s picture

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)

heyrocker’s picture

Issue tags:+Configuration system
Jose Reyero’s picture

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.

tim.plunkett’s picture

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

tim.plunkett’s picture

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

Jose Reyero’s picture

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

sun’s picture

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)

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

Working on this.

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new55.29 KB
FAILED: [[SimpleTest]]: [MySQL] 49,971 pass(es), 164 fail(s), and 63 exception(s).
[ View ]

I broke something to do with rdf and translation, but everything else seems to be working.

tim.plunkett’s picture

StatusFileSize
new57.76 KB
FAILED: [[SimpleTest]]: [MySQL] 50,035 pass(es), 9 fail(s), and 9 exception(s).
[ View ]

This should fix the translation tests.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
StatusFileSize
new896 bytes
new59.37 KB
PASSED: [[SimpleTest]]: [MySQL] 50,028 pass(es).
[ View ]

And that fixes the RDF tests!

yched’s picture

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.

tim.plunkett’s picture

+++ b/core/includes/entity.incundefined
@@ -54,18 +54,71 @@ function entity_info_cache_clear() {
+function entity_get_bundles($entity_type = NULL, $fallbacks = TRUE) {
...
+  return array();
...
+function entity_get_view_modes($entity_type = NULL) {
...
+  return array();

That's exactly what we do for unknown entity types, in both functions.

+++ b/core/includes/entity.incundefined
@@ -54,18 +54,71 @@ function entity_info_cache_clear() {
+      if (!isset($bundles[$type])) {
+        $bundles[$type][$type]['label'] = $entity_info['label'];

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:

  • The results of entity_get_info() no longer have any info about bundles or view modes
  • entity_get_bundles() has a different return value, for the old behavior you can do array_keys(entity_get_bundles($entity_type));
  • field_info_bundles() is replaced by entity_get_bundles(), they are effectively identical
yched’s picture

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

tim.plunkett’s picture

StatusFileSize
new59.37 KB
PASSED: [[SimpleTest]]: [MySQL] 50,510 pass(es).
[ View ]

Rerolled for blocks as plugins

Berdir’s picture

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

Berdir’s picture

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.

Berdir’s picture

Status:Needs review» Needs work
tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
new59.82 KB
FAILED: [[SimpleTest]]: [MySQL] 50,606 pass(es), 47 fail(s), and 6 exception(s).
[ View ]

The interdiff ignores whitespace, since its 90% indenting.
I figure entity_info_cache_clear() is still a reasonable place to put those.

Status:Needs review» Needs work

The last submitted patch, bundles-view-modes-1822458-33.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new62.7 KB
FAILED: [[SimpleTest]]: [MySQL] 49,393 pass(es), 51 fail(s), and 6 exception(s).
[ View ]

Adjusted that drupal_static check

Status:Needs review» Needs work

The last submitted patch, bundles-view-modes-1822458-35.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new60.65 KB
PASSED: [[SimpleTest]]: [MySQL] 49,558 pass(es).
[ View ]
amateescu’s picture

Status:Needs review» Needs work
Issue tags:-Configuration system
+++ 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.

jibran’s picture

Issue tags:+Configuration system

re tagging.
Edited: Sorry for re tagging.

amateescu’s picture

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

tim.plunkett’s picture

Issue tags:-Configuration system
StatusFileSize
new4.31 KB
new61.01 KB
PASSED: [[SimpleTest]]: [MySQL] 50,654 pass(es).
[ View ]

Okay, 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()

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Vocabulary.phpundefined
@@ -26,12 +26,6 @@
- *   },
- *   view_modes = {
- *     "full" = {
- *       "label" = "Taxonomy vocabulary",
- *       "custom_settings" = FALSE
- *     }
  *   }

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

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.

sun’s picture

Status:Needs work» Needs review
+++ 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).

tim.plunkett’s picture

StatusFileSize
new17.32 KB
new60.71 KB
PASSED: [[SimpleTest]]: [MySQL] 50,691 pass(es).
[ View ]

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

sun’s picture

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.

tim.plunkett’s picture

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

plach’s picture

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.

sun’s picture

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

tim.plunkett’s picture

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

sun’s picture

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.

tim.plunkett’s picture

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

EclipseGc’s picture

Status:Needs review» Reviewed & tested by the community

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

Eclipse

sun’s picture

Status:Reviewed & tested by the community» Needs work

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

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
new57.69 KB
PASSED: [[SimpleTest]]: [MySQL] 50,677 pass(es).
[ View ]

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.

plach’s picture

@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.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

#53 defeats the entire purpose of why I bothered to work on this issue, but it's better than an edit war. Thanks @effulgentsia.

plach’s picture

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

Crell’s picture

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.

fago’s picture

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.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

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

swentel’s picture

Title:Move dynamic parts (view modes, bundles) out of entity_info()» Change notice: Move dynamic parts (view modes, bundles) out of entity_info()
Priority:Major» Critical
Status:Fixed» Active
Issue tags:+Needs change record

This needs a change notice.

tim.plunkett’s picture

Status:Active» Needs review

Added change notice here http://drupal.org/node/1901332

tstoeckler’s picture

Title:Change notice: Move dynamic parts (view modes, bundles) out of entity_info()» Move dynamic parts (view modes, bundles) out of entity_info()
Priority:Critical» Major
Status:Needs review» Fixed

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.

Status:Fixed» Closed (fixed)

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

xjm’s picture

Issue tags:-Needs change record

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