Lazy-rebuilding the defaults for exported entities might not be desirable

I don't understand at all what the problem here is. Please elaborate.

As I see it, this is most likely not a bug in the Search API, but rather in Features, core or in whatever code creates the node. It seems that the node is first inserted, then the index is created and only then hook_entity_insert() is invoked for the node. Of course this would lead to problems, as it violates the contract for the insert hook.

So probably you should try to debug this further, look at the control flow and find out where the problem might lay.

In not making this throw an exception and bring everything to a halt, this would probably be another point for #1253320: Improve error handling. I should be far more cautious in throwing exceptions from hook implementations.

Ah, OK, now I understand the problem! Sorry for being so slow. ;)

- Node is created, hook_entity_insert() is invoked.
- Hence, search_api_entity_insert() is called.
- There, indexes are loaded.
- Features/Entity API sees the new index in code and calls hook_search_api_index_insert() for it.
- Tracking for the index is started, of course already including the new node.
- Code in search_api_entity_insert() continues, trying to add the node again to tracking.
- Chaos ensues.

I guess the proper fix here is to include something like your patch on a more generic level in the Entity API (or in Features – I don't know the code distribution between the two in this respect that well) – this is bound to happen with other exportable-providing modules, too, I guess. Or, you could include the fix in the code which creates the node, if that is custom code.
Also, the above issue would at least remove the exception, and you could probably live with an unnecessary watchdog entry more easily.

Yep, you are right, let's move this to the Entity API.

@fago, let's discuss the lazy-rebuilding of the defaults. The problem we have here is that Search API does some processing when an exported entity is created, and this can happen at a very inconvenient time because of the lazy-rebuilding of the defaults.

In our case, it happened after a node_save():

node_save
  - hook_entity_insert()
    - search_api_entity_insert()
      - search_api_track_item_insert // This loads the index (an exportable entity)
        - EntityAPIControllerExportable::load()
          - _entity_defaults_rebuild() // This entity_save() the new index.
            - search_api_search_api_index_insert()
               - SearchAPIIndex::queueItems() // This initialize the datasource with nodes from the system, including the one that we are saving because it already exists in the database.
        - SearchApiEntityDataSourceController::trackItemInsert() // This tries to insert the node we are saving into the datasource, but it fails because it is already there.

I see two options:

• As a quick workaround, we make sure to rebuild the default as soon as a module is enabled (see patch in #1), this is the most common situation this type of thing can occur.
• Or we allow an entity to specify that its defaults should be rebuilt only and immediately when entity_defaults_rebuild() is called, so as to avoid entities being rebuilt at inconvenient times.

Thoughts?

Ouch.

Currently defaults get rebuilt on cache-clear + on module install and fixed on uninstall. They are not immediately rebuilt though, but just marked to be rebuilt as soon as needed, i.e. on entity-load(). We could certainly immediately rebuild the entities provided in code when modules are enabled, however what's about the cache-clear case?
I'm a bit worried that rebuilding *all* exportable entities immediately upon cache clear might be too much. Also, it might in consequence trigger immediate cache-rebuilds of modules, what wouldn't be ideal on cache-clear either.
Of course we could introduce an option to disable cache-clear rebuilds for modules. That would probably require people to implement update hooks for changes to take affect then... ? Thoughts?

Just ran into an issue caused by #1273046: don't call rules_invoke_event() for configuration in conjunction with this. I must agree that having entity_load() possibly triggering an entity_save() is really not optimal.

Alternatives?
Just do it immediately on cache-clear? That would result in an immediate cache-rebuild of entity-info, what's not optimal either. However, looking at drupal_flush_all_caches() it does a menu_rebuild() before invoking the hook, so it probably has been already built for the menu anyway...

ok, here is a patch implementing the alternative method, i.e. immediately rebuild on cache-clear. The patch misses documentation updates and polishing, but suffices to give it a try.

Please test it on your sites, in particular on sites with lots of exportable entities in code and report whether it works for you.

subscribing

Probably it's a good idea to do that, so here is a more polished patch. Seems to work for me, please test!!

(in particular on sites with lots of exportable entities in code)

Works fine on some sites -> Committed.

This commit seems to have caused #1316140: Default rule configurations are no longer being rebuilt for tax types / rates and related #1321564: Delay in creating rules component menu links?.

I reverted Entity API commit e502b408 (this one) on the testbot and the tests ran fine.

@rfay: I've responded there, so setting this back to closed.

Add precisions