Hi, currently entity translation is integrated with pathauto only for the "node" entity; It would be nice to have integration with other core entities, "user" and "taxonomy term".

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

peximo’s picture

Attached patch should add this feature.

peximo’s picture

Status: Active » Needs review

Wrong status.

bforchhammer’s picture

Status: Needs review » Needs work

Thanks for your patch!

I'm wondering whether this can be done in a more generic way without any code specific to nodes (or taxonomy, or users)... e.g. use context module key as the entity type, replace entity_translation_node with entity_translation_enabled_bundle, ... ?

+++ b/entity_translation.module
@@ -1871,9 +1871,11 @@ function entity_translation_entity_uuid_presave(&$entity, $entity_type) {
+  $entity_types = array('node' => 'node', 'user' => 'user', 'taxonomy_term' => 'term');

Why do we need this mapping? Maybe add a code-comment.

+++ b/entity_translation.module
@@ -1871,9 +1871,11 @@ function entity_translation_entity_uuid_presave(&$entity, $entity_type) {
   // Ensure that we are dealing with a node having entity translation enabled.

This comment needs to be updated to reflect the additional entity types.

peximo’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Hi, I rewrited the patch by following your instructions; the mapping between entity type and token type can be done using the key "token type" in the entity info, I think it's better that way (I have adopted the same approach in a patch for the title module).
We need this mapping because taxonomy use "term" and not the entity type as token key.

plach’s picture

+++ b/entity_translation.module
@@ -1871,9 +1871,19 @@ function entity_translation_entity_uuid_presave(&$entity, $entity_type) {
+  // Ensure that we are dealing with a bundle having entity translation enabled.
+  if ($context['op'] == 'bulkupdate' && entity_translation_enabled($context['module']) && entity_translation_enabled_bundle($context['module'], $context['type'])) {

The mapped value needs to be used also here and replace $context['module']. However I'm a bit concerned about this approach because there is no guarantee that $context['type'] holds a bundle name, correct?

plach’s picture

Status: Needs review » Needs work

Actually there is also no guarantee that $context['module'] holds a value mappable to an entity type, hence at very least we need a check for that as in the Title patch.

Overall, I think that caring only about core entities and leaving the rest to modules implementing this hook would be an acceptable solution too.

peximo’s picture

Status: Needs work » Needs review
FileSize
2.79 KB

Ok, I added a check on the mapping and rewrited the extraction of entity type and bundle to make it safer.
I checked also that entity_translation_enabled() and entity_translation_enabled_bundle() don't generate errors or warnings if the entity_type and/or the bundle is invalid.

peximo’s picture

The mapping is no longer necessary, is the same as the entities info. Also isset($info[$context['module']]) implicitly checks that $context['module'] is an entity type.

plach’s picture

Looks good to me, thanks. One minor thing: can we have an $entity_type variable instead of $context['module'] to improve readbility?

plach’s picture

Status: Needs review » Needs work
+++ b/entity_translation.module
@@ -1871,19 +1871,25 @@ function entity_translation_entity_uuid_presave(&$entity, $entity_type) {
+  if ($context['op'] == 'bulkupdate' && isset($info[$context['module']])) {

To be 100% safe I think this should be !empty($info[$entity_type]['token info']) && !empty($context['data'][$info[$entity_type]['token info']])

bforchhammer’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

Updated patch with changes suggested in #9 and #10.

peximo’s picture

@bforchhammer: the key is not "token info" as written by Plach but "token type".
Rerolled.

peximo’s picture

Sorry wrong patch.

plach’s picture

Status: Needs review » Postponed
Issue tags: +Needs tests

I guess we need some tests here but they should not block commit (RTBC from me).

However I'd like to commit this after beta2 is out to minimize the risk of introducing new bugs.

plach’s picture

Status: Postponed » Reviewed & tested by the community

Now the beta2 is out we can commit this.

bforchhammer’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed and pushed :)

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

  • Commit 5933679 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions authored by peximo, committed by bforchhammer:
    Issue #1842540 by peximo, bforchhammer | plach: Added Pathauto core...

  • Commit 5933679 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions, workbench authored by peximo, committed by bforchhammer:
    Issue #1842540 by peximo, bforchhammer | plach: Added Pathauto core...