Allow for multiple base paths

The per-bundle path approach looks reasonable to me. Patches welcome.

This will probably needed to fix #1470018: Provide Entity Translation integration. Therefore we will need just multiple arrays, per-bundle base paths wouldn't work in that case.

I think we should allow the 'base path' key to be an array, but keep the possibility to have a single string value for BC. The base path is also used as default to construct the edit and view paths (which are used to generate UI links), which instead need to be unique. I'd just choose the first available base path in that case.

plach: I think you should start looking at entity_translation.api.php for the description of the keys and then look at entity_translation_menu_alter() to see how the 'base path' key is used. [...]

Attached is a first stab at allowing the 'base path' to be an array while leaving edit and view paths unique. Not sure how to test this or apply it for #1470018: Provide Entity Translation integration though...

Don't we need multiple edit paths for a localized form on media/%file/edit/%nojs (i.e. in the "edit media" popup)?

@bforchhammer

Thanks for the patch!

Don't we need multiple edit paths for a localized form on media/%file/edit/%nojs (i.e. in the "edit media" popup)?

Yes, exactly. We need to support both the regular file/%file and the media/%file/edit/%nojs popup path.

From a cursory look the code looks code: I'd say it needs manual testing, however setting to NR to see if the bots complains.

Yes, exactly. We need to support both the regular file/%file and the media/%file/edit/%nojs popup path.

So, just to clarify: we DO need to allow/support arrays for edit path (and view path?) as well? Patch above only allows the base path to be an array... ;)
If yes, should the translation handlers also know about all paths, or just the "first one"? What aspects need to remain unique?

Here's a new patch which allows arrays for 'base path', 'edit path', 'view path' and 'path wildcard'. Our implementation of hook_entity_info_alter() makes sure they are always stored as array values in the entity info array.

Now, if I add the additional media edit path the form automatically shows up translated when I click "edit media" :) Everything else also still seems to be working...

API docs still need fixing, and I left in code which adds the additional media edit path.

+++ b/entity_translation.module
@@ -108,19 +108,32 @@ function entity_translation_entity_info_alter(&$entity_info) {
+      foreach (array('base path', 'edit path', 'view path', 'path wildcard') as $key) {

So, the question remains: does it make sense to make the 'view path' key an array? I guess it's possible that there are multiple view paths, but I don't think entity translation needs to know about it?

+++ b/entity_translation.module
@@ -108,19 +108,32 @@ function entity_translation_entity_info_alter(&$entity_info) {
+      $edit_path = array_map(function($s) { return $s . '/edit'; }, $view_path);

Using a closure adds a dependency on PHP version >=5.3. Is that okay?

+++ b/entity_translation.module
@@ -108,19 +108,32 @@ function entity_translation_entity_info_alter(&$entity_info) {
+      // @todo Move to media module (or somewhere else).
+      if ($entity_type == 'file') {
+        $edit_path[] = 'media/%file/edit/%ctools_js';
+      }

Obviously needs to go somewhere else... where?

+++ b/entity_translation.module
@@ -191,104 +204,112 @@ function entity_translation_menu_alter(&$items) {
+        if ($delta == 0 && !isset($paths[preg_replace($regex, '%', $path)])) {
+          drupal_set_message(t('The entities of type %entity_type do not define a valid base path: it will not be possible to translate them.', array('%entity_type' => $info['label'])), 'warning');

Currently only throws an error if the first base path is invalid. Do additional base paths need to validated as well?

Here's a reroll of #9, as it didn't apply cleanly anymore...

Here's a new patch which fixes the issues mentioned in #10. I decided for making 'view path' an array for consistency, removed the php closure (D7 officially supports PHP <5.3), removed the media-specific code, added validation for all base paths, and added some basic documentation to the api file.

An aspect which I'm not quite happy with, is that this approach does not consider dependencies between the different path related elements; e.g. all path wildcards are stored in one array, even though they should only be used for the respective menu path. Theoretically this could lead to the wrong path wildcard being used to identify an entity... Similarly, $base_path/edit is currently always added as an edit path; in most cases this will be fine, but if some entity wants to use that path for something else then there's currently no way to prevent ET from overriding it.

Maybe it would be better to allow the definition of multiple "path sets", like e.g. the following:

function hook_entity_info() {
  $info['custom_entity_multiple_paths'] = array(
    'translation' => array(
      'entity_translation' => array(
        'class' => 'EntityTranslationCustomEntityHandler',
        'path sets' => array(
          array(
            'base path' => 'custom_entity/%custom_entity',
            'path wildcard' => '%custom_entity',
          ),
          array(
            'edit path' => 'fancy/%entity/edit',
            'path wildcard' => '%entity',
          ),
        ),
      )
    )
  );
}

Of course we'd have to make sure that the current (simpler) way for defining paths is still supported as well (in hook_entity_info_alter).

Other issues:

• Position calculations in hook_menu_alter() should probably use the path wildcard for determining the entity position, and count(explode('/', $path)) as base value for positions on sub-paths.
• Would it make sense to add a "translate path" element with a default value of $base_path/translate, to allow modification of the translate path?

The plan in #13 sounds good to me.

Would it make sense to add a "translate path" element with a default value of $base_path/translate, to allow modification of the translate path?

Not sure about this: I think that enforcing a bit of consistency would be better and the translation handler can always be replaced with a specialized version overriding the get*Path() methods in extreme cases. OTOH having a getTranslatePath() method would make sense. No strong preference here.

Anyway, I see another potential issue with the current approach: by defaulting to the first base path and friends we are basically ignoring context. For instance if we consider the original use case that spawned this issue (Media), we have two base paths:

file/%file

and

fancy/popup/whose/path/I/don-t/remember/exactly/%fancy_popup_file

When displaying the translation overview page at

fancy/popup/whose/path/I/dont/remember/exactly/%fancy_popup_file/translate

all links will point to file/%file and file/%file/edit. And if we want to skip the translate tab, which probably would not appear, and just provide the language switcher tabs we would incur in the same problem, I guess. This make me think we probably need to default to the current router path if it matches one of the base paths. We should set the default base path through a method, default to the current router path if none is provided and fall back to the first one if it does not match any defined base path.

+++ b/entity_translation.module
@@ -189,104 +214,118 @@ function entity_translation_menu_alter(&$items) {
+        if(!isset($paths[preg_replace($regex, '%', $path)])) {

Missing space after "if".

Attached is a new patch which...

• adds a "path schemes" element to the entity info array as outlined above. Default (old) path elements are automatically added as a "default" path scheme in hook_entity_info_alter().
• moves "path scheme validation" into a separate function (and performs additional validation)
• adds EntityTranslationHandlerInterface::switchPathScheme() to allow switching of the currently active path scheme... this may become useful for the context-switching issue mentioned in #14.
• adds EntityTranslationHandlerInterface::getTranslatePath() as outlined in #13; also cleaned up path usage in entity_translation.admin.inc and switched to getTranslatePath() where possible.

Let's see whether I've broken anything. Go testbot.

And again...

Awesome! I made some code clean-up and removed special-casing for the node admin theme.

+++ b/includes/translation.handler.inc
@@ -257,11 +273,9 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
+    $this->activePathScheme = 'default';

I think we still need to set the active patch scheme based on the current router path.

Setting NR for the bot but it's still NW.

Attached patch adds code to find the active path scheme based on the value of $_GET[q].

Not sure whether this is the best approach; we should probably try to reuse more core-menu functionality, however using e.g. menu_get_item() to get the active router does not work as it leads to an infinite loop... (When I tested it, the translation handler was constructed during entity_translation_field_language_alter(), which itself is invoked via the entity_load() call during menu_get_item()).

Nice! The attached patch slightly changes the initialization of the path scheme based on the router path: I think it's better to explictly initialize path schemes. The router map obtained this way let us deal with multiple loaders/wildcards inside the same router path. This also avoids the recursion problems mentioned above.

I actually tested the patch with Media (#1418644-7: Add multilingual support for files and #1470018-8: Provide Entity Translation integration) and it seems to work fine. I think it's ready to go in, if you are ok with it.

Yeah, that's looking good :)

I'll commit #22 with some minor changes in a minute...

Go!

Committed and pushed the attached patch :)

Whoo-hoo, beta1 approaching!

Let's if we can get some feedback in the File entity/Media issues. Review/testing of patches would be welcome: let's get them RTBC!

Related issue: #1676716-6: Do not list entity types that do not integrate with ET in the admin page.

Note that at least on existing installs, this patch causes a notice, a warning and an exception:

- Notice: Undefined index: path schemes in entity_translation_admin_paths() (Zeile 610 von /home/web7/html/drupal/sites/all/modules/entity_translation/entity_translation.module).
- Warning: Invalid argument supplied for foreach() in entity_translation_admin_paths() (line 610 of /var/www/virtual/iranomid/html/sites/all/modules/entity_translation/entity_translation.module).
- Exception: Cannot initialize entity translation path variables (invalid path scheme). in EntityTranslationDefaultHandler->initPathVariables() (line 1200 of /var/www/virtual/iranomid/html/sites/all/modules/entity_translation/includes/translation.handler.inc).

Reversing the patch helped on both installs.

Reopening this as a critical bug to allow for a quick followup. To be demoted to a normal task before closing.

Just clear the caches.