Using hook_entity_info_alter to add the bundles prevents some modules from altering entities

This seems like a hacky way of creating bundles unless you mean to alter bundles of an entity type created by another module. See a core example of bundle creation: http://api.drupal.org/api/drupal/modules--field--tests--field_test.entit...

hm, yep we need to use the alter hook because of the recursion issue.

>For instance, because RDF module uses rdf_entity_info_alter to attach the RDF mappings, any module which is after rdf_ in hook order will not have RDF mappings defined for bundles it declares. This can be changed by altering the order of the hook implementations, but that seems a bit like a hack.

That's due to the way altering works and would apply to any bundle added via the alter hook. If RDF module wants to be sure it gets *all* bundles, even those added in via alter, it shouldn't rely on the information already there during altering. Instead, maybe use entity_get_info() to create mappings, store them in db, and then just add-in all mappings during entity info alter.

The doxygen for that function is almost more important than its code itself, as it warns that developers should not use hook_entity_info_alter() to alter RDF mappings!

/**
 * Implements hook_entity_info_alter().
 *
 * Adds the proper RDF mapping to each entity type/bundle pair.
 *
 * Modules willing to alter the bundle mappings should not implement this hook,
 * but instead use the RDF CRUD mapping API which will store the mappings in the
 * rdf_mapping table.
 */
function rdf_entity_info_alter(&$entity_info) {
  // Loop through each entity type and its bundles.
  foreach ($entity_info as $entity_type => $entity_type_info) {
    if (isset($entity_type_info['bundles'])) {
      foreach ($entity_type_info['bundles'] as $bundle => $bundle_info) {
        if ($mapping = _rdf_mapping_load($entity_type, $bundle)) {
          $entity_info[$entity_type]['bundles'][$bundle]['rdf_mapping'] = $mapping;
        }
        else {
          // If no mapping was found in the database, assign the default RDF
          // mapping for this entity type.
          $entity_info[$entity_type]['bundles'][$bundle]['rdf_mapping'] = _rdf_get_default_mapping($entity_type);
        }
      }
    }
  }
}

If the rdf-info should not be touched in hook_entity_info() I wonder why it is there at all?

Anyway, basically changing the code to do a
>_rdf_mapping_load($entity_type)
which gets all mappings for all bundles known to the rdf module, should solve it.

> If the rdf-info should not be touched in hook_entity_info() I wonder why it is there at all?

What do you mean? can you clarify? rdf.module does not implement hook_entity_info()

Ad #7, yep indeed that's not ideal. We could definitely change it to do merging. Still, rdf.module would have to ensure it only stores info for bundles that are really present.

ad #6:
I just wondered, if the rdf module information in hook_entity_info (or its alter variant) should not be altered there, why is it there at all?

> I just wondered, if the rdf module information in hook_entity_info (or its alter variant) should not be altered there, why is it there at all?
because we need to add this info into the entity_info cache at some point, when clearing the cache. the rdf.module alter entity_info to put the data from its rdf_mapping table, that saves us from querying the rdf_mapping table for every page request, since it's cached in entity_info.

I see. So maybe the solution outlined in #7 would be the best we can do then?

The code in entity-test and others could be changed to something like
>$entity_info['entity_test']['bundles'][$name]['label'] = info->label;

It comes to my mind that the taxonomy module uses the same way to add in its bundles. Does it work there?

Anyway, fuddling with the module weights / module implements callback order should be possible workarounds. Though, it might be a good idea to make rdf.module more reliable in core.

@fago: taxonomy.module registers its bundles in taxonomy_entity_info(). I do not see any implementation of hook_entity_info_alter() in the core taxonomy module.

ops sry, looks like that got changed some time ago.

of course, but how does this relate to this issue?

It's totally valid to add bundles during hook_entity_info_alter() - you may arbitrarily change the entity-info during alter. That's what the hook is for.
If rdf.module can't cope with that, it's a bug there. Still, an interim workaround is fixing the weights such that the rdf.modules comes last/after the module in question.

It's totally valid to add bundles during hook_entity_info_alter()

Actually, that's a misconception in core. Alter hooks are not supposed to add stuff, they are supposed to change already added stuff.

The problem is that hook_entity_info() registers both entity types and bundles. Whereas types need to exist before bundles can be added, so any module that merely wants to add bundles has a dependency on types being added already.

Further, generally enhancing modules like RDF rely on all types and all bundles being registered already; they are the typical alter hook use-case, because they possibly manipulate all existing entries.

Thus, core is clearly missing an intermediate step - i.e., it should call hook_entity_info(), then hook_entity_info_additions(), and only lastly, hook_entity_info_alter().

For D7, we might have to implement hook_module_implements_alter() for RDF module to ensure that it's invoked last for hook_entity_info_alter(). However, even that approach is limited, since hook_module_implements_alter() is a hook itself, so any module that's invoked after RDF module may do the same tweak as RDF module, so in turn, rdf_entity_info_alter() will no longer be invoked last.

For not wanting to add additional cache_get()s on every request, I opened #1187752: Add a shared cache recently. That would allow both entity info and RDF mappings to piggyback on a single cache_get(), without having to use hook_entity_info_alter().

>Actually, that's a misconception in core. Alter hooks are not supposed to add stuff, they are supposed to change already added stuff.

It doesn't add an entity-type, but changes the info of an existing one. So I think that's fine.

>The problem is that hook_entity_info() registers both entity types and bundles.
Indeed.

>Thus, core is clearly missing an intermediate step - i.e., it should call hook_entity_info(), then hook_entity_info_additions(), and only lastly, hook_entity_info_alter().

I'm not sure about that. Why not move the additions to separate hooks? That way we have no problem with depending on the available entity types + I don't think we want a big fat entity-info array that contains possible everything. So better split up info-arrays in fitting chunks. That potentially helps to an improve the memory footprint too.
Thus what about adding something like hook_entity_bundle_info() for d8? But well, that discussion should probably be another issue ;)

It doesn't add an entity-type, but changes the info of an existing one. So I think that's fine.

Point being, rdf_entity_info_alter() only adds information and doesn't change existing. That's the domain of info hooks, not alter hooks. Just simply consider a module that wants to alter the additions of RDF module...

Why not move the additions to separate hooks?

I don't think changing the amount of data will improve any of the logical invocation problems. Regardless of whether you collect the info in one big or many small chunks, we're still missing a second stage of additions before the alter hook is invoked. This is a general design misconception in core and applies to all info + alter hook scenarios. And we have a core issue a possible fix could be added to: #890660: add an alternative way of doing module_invoke_all() which sets a 'module' key in the returned info

Attached patch clarifies what I mean. Quite a hack, but works solidly.

>I don't think changing the amount of data will improve any of the logical invocation problems. Regardless of whether you collect the info in one big or many small chunks, we're still missing a second stage of additions before the alter hook is invoked.

If there would be a separate rdf-entity-info hook, it could safely depend on entity-info. Thus yes, it would solve the invocation problems.
(There is a separate rdf hook, but it runs before and it is without alter and cache. So rdf.module piggy-backs on hook_entity_info() for that.)
The same way it would work for bundles. If there is a separate hook for it, it would be safe to depend on entity-info, i.e. to call entity_get_info() from a bundle-hook implementation.

So I don't think that a general second step buys us much, it just postpones the same troubles by one step. As soon as a module again uses alter (e.g. to alter additions), we run into the same problem again as we are not able to properly alter the alterations. Consider a module uses the additions-stage to add a bundle, then rdf.module would have to use alter and again we'd have no possibility to alter rdf.module info.
Compared to that, moving chunks that depend on entity-info out of entity-info would create a separate stage for each chunk moved out and provide a clean per-hook possibility to alter the info. Thus the rdf-hook could depend on the bundle hook, which depends on the entity-info hook + the same way for any other related hook. For altering, each hook can use drupal_alter() on its own, so subsequent hooks would automatically take alterations into account.

Moving for example bundle info out of hook_entity_info() would fix the infinite recursion issue with taxonomy vocabularies and taxonomy_entity_info(), we might not want to have taxonomy vocabularies as entities in D8, but if we kept that I'd be glad to see it less of a headache than it currently is.

The problem popped up again, so it's time to finally do away with the entity-load interdependency - see #1290986: avoid entity_load() during entity info cache rebuilds (input welcome!). This would fix this one too, so I'm marking this as duplicate.