I just recognized that the call of menu_get_router() in entity_translation_entity_info_alter() causes some problems. Due to that, entity_get_info() is called, before it could be fully built.

So in my use case, entity.module could not set the _entity_info_add_metadata()-Callbacks. That leads to missing actions for rules as rules_entity_type_options() is empty on rules cache building.

I attached a Call-Stack for the nested call.

Comments

plach’s picture

Version: 7.x-1.0-alpha1 » 7.x-1.x-dev

I'm afraid the only way to fix this is defaulting to the 'entity_type/%entity_type' pattern, if the 'base path' tranlsation information is missing, without checking it's a valid router path. I think this might not be a huge problem after all.

donquixote’s picture

What I don't understand, what is this 'base path' used for after all?
Can't we postpone finding that thing to a later stage of the game?

donquixote’s picture

Ok, looks like it is used in hook_menu().
That's the old chicken-egg problem of menu router building, if one path depends on another.

Solution is to use hook_menu_alter(), if you really want to let your own router items depend on other router items.
Otherwise, just let those other modules implement hook_translation_info(), so they can tell entity_translation about those paths.

donquixote’s picture

I made quite a radical patch.
Unfortunately, I started with the 7.x-1.0-alpha1, and now that is quite difficult to merge into the -dev :(
Stupid me.

I will upload the patch for 7.x-1.0-alpha1 now, before I attempt to merge anything.

donquixote’s picture

And now the same thing against 7.x-1.x (or "master", as it is named in git)

donquixote’s picture

Status: Active » Needs review

Explanation:

The idea of the patch is, we cannot know the correct paths for a given entity type during hook_entity_info_alter(), because that might already be during or directly before a menu rebuild.

The best moment for us to guess those paths is during hook_menu_alter(). There we can check for existing paths, and the result is directly written to menu router. I made a nice heuristic for that, which is probably better than what was there before.

Now, aside of menu rebuilding, we need the same path information in other places, for links display.
We could extract this information from the current contents of the menu_router table, but
(a) we don't want to trigger a menu rebuild, and
(b) we can't be sure that the result of this new extraction heuristic will be the same that we got in hook_menu_alter(), and
(c) what if the menu is rebuilt, then this information would be out of date. (probably still ok, since mostly there is a redirect after menu rebuild - is it?)

Thus, it is a good idea to remember the result of our extraction heuristic from hook_menu_alter(), make it available to those components that need it, and update it on menu rebuild.

This is what the PathInfoRegistry is supposed to accomplish.

---------

The goal of this patch was to produce a working solution, with the correct algorithms in place.
There is still some decisions to make about which class and function goes where, how they should be named, etc.
I leave this up to you :)

donquixote’s picture

Btw, I suppose this issue causes a lot of other bugs as a side effect.
Maybe a lot what is currently in the issue queue, is actually caused by this beast.

plach’s picture

@donquixote:

Thanks for working on this issue, but honestly I ain't sure it deserves such an effort. Perhaps requiring a valid base path should be enough for most use cases, instead of adding all this new code which will have to be maintained (and understood ;)

donquixote’s picture

Thanks for working on this issue

I needed something that works.
Crazy I ended up with sth like this. But it does work.
(could still be a small bug somewhere, but it is a robust idea)

requiring a valid base path

Ok, so.. we stop guessing a path from hook_menu(), and instead let other modules define a path explicitly? Where would this happen?
Esp, entity types defined by modules that don't bother to support entity_translation..

donquixote’s picture

Hm, I guess this would happen via hook_entity_info() and hook_entity_info_alter().
Modules can use these hooks to suggest a base path, but they should do so without "cheating". That is, without requesting menu router info.

plach’s picture

Ok, so.. we stop guessing a path from hook_menu(), and instead let other modules define a path explicitly? Where would this happen?

Yes, exactly: hook_translation_info() already uses a 'base path' key, see entity_translation_translation_info().

Esp, entity types defined by modules that don't bother to support entity_translation..

Well, supporting ET is just matter of implementing a hook and return a small structured array, I don't see it as a huge requirement. Given that ET is likely to be part of D8 in some form, supporting it should not be a great issue. Moreover, it's easy to implement hook_translation_info() on behalf of other modules while waiting for them to add support for ET explicitly.

plach’s picture

Component: Code » Base system
plach’s picture

Status: Needs review » Needs work

Let's just remove the nested call as per #11.

plach’s picture

Status: Needs work » Needs review
StatusFileSize
new2.46 KB

The attached patch removes the nested call but leaves the default base path in place, the base path check is performed directly in entity_translation_menu_alter(). This should fix the issue without removing the ability of automatically support entities with "standard" base path.

torpy’s picture

As hinted in #1440476: Argument #2 is not an array in entity_translation_menu_alter(), this patch does fix that issue for me.

jonmcl’s picture

Patch #14 working well for me. Solved #1366712: Admin theme is no longer used after cache is cleared for me

plach’s picture

Status: Needs review » Fixed

Committed and pushed to the master branch.

Status: Fixed » Closed (fixed)

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

  • Commit f190c60 on master, et-permissions-1829630, factory, et-fc, revisions by plach:
    Issue #1341314 by plach , donquixote | derhasi: Fixed Nested call of...

  • Commit f190c60 on master, et-permissions-1829630, factory, et-fc, revisions, workbench by plach:
    Issue #1341314 by plach , donquixote | derhasi: Fixed Nested call of...