Support arbitrary base paths

Related issue: #1110980: Profile2: how to make the translate tabs appear.

Should I try to investigate or are you working on this already ?

No, not yet.

Read #1110980-20: Profile2: how to make the translate tabs appear for details.

I'm trying to study the possible solutions to this problem, but I think there are design decisions which have to be taken.

As of now, I don't see how complex paths could be handled in a universal way.

The module has to guess what type of entity it is handling, and with a path like user/%user/edit/some_other_entity/%entity_id, I don't think there is a logical solution to handle all possible cases. Here, how could the module know if it should translate the user or the "some_other_entity" ?

Another example : in the path mentionned by the OP, there is no indication except common sense that the entity to translate is a survey and not a path.

So, maybe should there be a namespace convention settled on top of the current one which is very well done. From the top of my head, the rule would state something like one of the following :

- the entity type should always be at the same place (probably unaplicable)
- there should be a given order of arguments
- there should be a string in the path from which the relative position of the rest of the info could be guessed.

Or maybe some new entries in the translation array passed by hook_entity_info could help the menu hook to know where to find the information, replacing the position computation $entity_position = count(explode('/', $path)) - 1;

Something like :

function somemodule_translation_info($types = NULL) {
  $info['entity_type'] = array(
    'translation' => array(
      'entity_translation' => array(
        'base path' => 'user/%user/edit/%entity_type/%entity_id',
        'entity_position' => 3,
        'entity_id' => 4,
        'source_position' => 5,
        ...
      ),
    ),
  );
  return $info;
}

If the infos are not there, the menu hook could always fallback to the current standard used for nodes.

I would be happy to try to code this if you agree with this solution. I hope it doesn't sound too dumb. What do you think ?

@Countzero:

Initially I was leaning towards your position, but then I realized that in most situations the needed path data will be available in the current path through the arg function. Modules having more complex needs can always extend the base translation handler class and override the getPathInstance() method or the various get[Type]Path() methods.

The attached patch should allow any entity having multiple wildcards in their base path to work by simply setting 'path arguments' to TRUE in their entity translation information. The default is false to avoid unnecessary processing.

Please test and report. I tried with core entities and everything seems to be ok.

Edit: In the case of the OP the correct settings should be:

<?php
'survey_path' => array(
  'label' => 'Path',
  //  ...
  'translation' => array(
    'entity_translation' => array(
      'base path' => 'admin/surveys/%survey/paths/%survey_path',
      'access callback' => 'user_access',
      'access arguments' => array('administer surveys'),
      'edit form' => true,
      'path wildcard' => '%survey_path', // This line might not be needed
      'path arguments' => TRUE,
    ),
  ),
),
?>

I think the patch works OK but some work is needed (on the profile2 side) because profile2 handles the profile types by name rather than id. So I had to tweak a little, hard coding the profile type in the hook.

I don't know if other modules proceed the same way, but it should be possible to test the type of the $segments[$index] variable and call another method than getEntityId but I guess it would rise problems.

Except for this point, the patch seems to do its job. The way you handle the arguments is very neat. Before you posted it, I got to the point where I started to overload the Handler class in profile2, and I guess that's what I'll do in the end, but I missed the guts (and the time) to try to rewrite the getPathInstance method in et.

There's lots of technical talk about details here, but I'm missing a high-level problem description. What is the actual problem? What is expected? What didn't happen? Do these expectations apply to everything? If not, what would be exceptions to the rule?

The very same information is also missing in the patch, but before you start to add in-code docs and comments, we should discuss and get on track on-issue.

@sun:

ET tries its best to attach the translation UI (basically the translate tab) to any entity type's UI. Through the translation information, modules implementing new entity types can provide a base path: this is the path to which the translate tab is attached; the translation overview page can then be reached by appending the /translate segment to it.

If a base path is not provided ET defaults to the common pattern 'entity_type/%entity_type' and checks that a menu router item actually exists for it.

The base path is "instantiated" in the translation UI with the correct entity id. The problem we are facing here is that there might be entity types having base paths in which more than one wildcard appear (see the OP). The issue is all about trying to remove the assumption that only the %entity_type wildcard appears within the base path.

The base path is "instantiated" in the translation UI with the correct entity id. The problem we are facing here is that there might be entity types having base paths in which more than one wildcard appear (see the OP).

It's still not clear to me as to why that is a problem. I'm specifically concerned seeing arg() in this patch, which shouldn't be used anymore, and more importantly, only works when the currently requested page/path is the path of the translatable entity. Which in turn means that you cannot instantiate the entity translation handler class when the current request path is not the entity's path.

I think we should re-start by writing tests here; i.e., implement two simple entity types in entity_translation_test and make one of them use a router path that depends on the other.

It's still not clear to me as to why that is a problem.

It is a problem because we have to find somewhere the data to "instantiate" the path (see EntityTranslationDefaultHandler::getPathInstance): if the path only contains the entity wildcard it's easy, but when we have more than one wildcard it isn't.

I'm specifically concerned seeing arg() in this patch, which shouldn't be used anymore,

I asked your feedback exactly because I was slightly worried by it and I hoped you could suggest the right alternative.

and more importantly, only works when the currently requested page/path is the path of the translatable entity. Which in turn means that you cannot instantiate the entity translation handler class when the current request path is not the entity's path.

Yes, this is my other concern: in all the cases currently implemented this assumption is satisfied and everything works, but as soon as some custom code tries to instantiate the translation handler, the most we can get with this approach is avoiding to instantiate the UI paths, which is ugly.

I think we should re-start by writing tests here; i.e., implement two simple entity types in entity_translation_test and make one of them use a router path that depends on the other.

I'd prefer to discuss a working solution before writing code: in the scenario you describe the entities might well have cross references in their data structures allowing getPathInstance to work without problems. But we can't assume this will always be true. My doubt is: is there a generic way to handle an arbitrary path in which any wildcard may appear or each entity type having such a path should provide its own getPathInstance implementation?

Since the current solution is not viable as sun pointed out in #11 and I cannot think of a cleaner one, I'm going to wontfix this. Feel free to reopen if you can suggest a valid alternative.

At the moment the only solution available is overriding EntityTranslationDefaultHandler::getPathInstance method.