Closed (fixed)
Project:
Entity Translation
Version:
7.x-1.x-dev
Component:
Base system
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
14 Nov 2011 at 18:51 UTC
Updated:
12 Jun 2014 at 22:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
plachI'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.Comment #2
donquixote commentedWhat 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?
Comment #3
donquixote commentedOk, 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.
Comment #4
donquixote commentedI 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.
Comment #5
donquixote commentedAnd now the same thing against 7.x-1.x (or "master", as it is named in git)
Comment #6
donquixote commentedExplanation:
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 :)
Comment #7
donquixote commentedBtw, 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.
Comment #8
plach@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 ;)
Comment #9
donquixote commentedI 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)
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..
Comment #10
donquixote commentedHm, 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.
Comment #11
plachYes, exactly:
hook_translation_info()already uses a'base path'key, seeentity_translation_translation_info().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.Comment #12
plach#1536474: Calling menu_get_router may break other hook_entity_info_alter() for other modules has a patch that can serve as initial work.
Comment #13
plachLet's just remove the nested call as per #11.
Comment #14
plachThe 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.Comment #15
torpy commentedAs hinted in #1440476: Argument #2 is not an array in entity_translation_menu_alter(), this patch does fix that issue for me.
Comment #16
jonmcl commentedPatch #14 working well for me. Solved #1366712: Admin theme is no longer used after cache is cleared for me
Comment #17
plachCommitted and pushed to the master branch.