According to Gabor we absolutely need a language selection widget on every entity edit form. I think this might be problematic in some situations, e.g. when creating terms through a node taxonomy field, hence we need some kind of fall back strategy.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach’s picture

Component: Code » Base system
Assigned: Unassigned » plach

Working on this.

plach’s picture

Status: Active » Needs review
FileSize
17.84 KB

Here is a first attempt. To do:

  • Extra field language widget reordering.
  • Granular settings to display the language selection widget per entity bundle.
  • Granular settings for the default entity language.

Let's see if the bots complains.

plach’s picture

Implemented per-language settings. Only field reordering is missing.

plach’s picture

Et voilà. I think now we need only an update function to keep BC.

bforchhammer’s picture

I'll try to test+review this soon... :)

bforchhammer’s picture

Status: Needs review » Needs work
FileSize
25.12 KB

Patch #5 summary of changes:

  • It adds a "language selector" for all entities, e.g. comments, files (tested with file_entity), bean, ...
  • It adds new bundle-specific settings for a) setting the default language, and b) hiding the language selector.

This is the new settings screen:

The new settings are currently also applied to entities which have ET disabled! Is that intended?

Patch #5 code review

+++ b/entity_translation.admin.inc
@@ -57,6 +57,65 @@ function entity_translation_admin_form($form, $form_state) {
+  $et_settings = variable_get('entity_translation_settings', array());

I think that this variable should be split into separate entity_translation_settings_TYPE__BUNDLE variables; having only one variable makes it hard to e.g. export settings for only a few selected content types. There was a similar issue in core with the field_bundle_settings variable, see this change notice.

It would be cool if it was also possible to provide global or entity defaults, and override them on the bundle level. (Personally I'd probably almost always use "current language" and "show selector".) However, as far as I know core doesn't do something like this anywhere either, so definitely not required.

+++ b/entity_translation.admin.inc
@@ -57,6 +57,65 @@ function entity_translation_admin_form($form, $form_state) {
+    'xx-et-default' => t('Default language'),
+    'xx-et-current' => t('Current language'),
+    'xx-et-author' => t('Author language'),

The keys could (should?) be moved into constants so that we don't have to juggle strings...

+++ b/entity_translation.module
@@ -351,6 +352,10 @@ function entity_translation_menu_alter(&$items) {
+          if ($item['access callback'] == 'entity_translation_tab_access') {
+            $item['access arguments'][] = $entity_position;
+          }

@@ -642,8 +647,12 @@ function entity_translation_admin_paths() {
+function entity_translation_tab_access($entity_type, $entity) {
+  if (drupal_multilingual() && (user_access('translate any entity') || user_access("translate $entity_type entities"))) {
+    $handler = entity_translation_get_handler($entity_type, $entity);
+    return $handler->getLanguage() != LANGUAGE_NONE;
+  }
+  return FALSE;

+++ b/entity_translation.node.inc
@@ -65,7 +65,7 @@ function entity_translation_node_tab_access() {
+      return entity_translation_tab_access('node', $node);

Are these changes required for the language selector? What's their effect on the permissions tests in #1770750: Improve test coverage?.

+++ b/entity_translation.module
@@ -1222,8 +1259,16 @@ function _entity_translation_element_title_append(&$element, $suffix) {
+    // We need to process the posted form as early as possible to update the
+    // form language value.
+    array_unshift($form['#validate'], 'entity_translation_entity_form_validate');

+++ b/includes/translation.handler.inc
@@ -1003,10 +1058,6 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
-    // We need to process the posted form as early as possible to update the
-    // form language value.
-    array_unshift($form['#validate'], 'entity_translation_entity_form_validate');
-
     // Process entity form submission.
     $form['#submit'][] = 'entity_translation_entity_form_submit';

These changes make me wonder whether it would be good for developers to attach all ET form handlers in one place... so maybe we should also move the #submit handler into hook_form_alter()? Not sure.

+++ b/entity_translation.module
@@ -1470,7 +1523,8 @@ function entity_translation_language($entity_type, $entity) {
+ *   (optional) The entity to be translated. A bundle name may be passed to
+ *   instantiate an empty entity.

This change is causing notices with the bean module (bean translation handler), because it relies on having a properly loaded entity: Undefined property: stdClass::$delta in EntityTranslationBeanHandler->getEntityId() This will need a follow-up issue...

Maybe we should also add a respective comment to the handler interface docs, saying that the entity might only be a stub entity.

+++ b/includes/translation.handler.inc
@@ -837,6 +905,11 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
+    // Store the information about being editing the original values for later
+    // reuse: a language change might render impossibile to determine this
+    // otherwise.

Not sure what this comment is trying to tell me :)


@plach: great work on the patch! I can see the beta release coming closer and closer... :)
bforchhammer’s picture

Another minor todo: the new entity_translation_settings variable should be deleted in hook_uninstall().

plach’s picture

Status: Needs work » Needs review
FileSize
32.38 KB

I think we should be ready to go now.

plach’s picture

Er, I missed #7. Hopefully some of the issues were fixed by #9. Here is an interdiff.

Basically I added an update function and new entity info stuff to provide better BC. BTW now we have per-bundle translation.

plach’s picture

This plus #9 should fix #7 except for:

It would be cool if it was also possible to provide global or entity defaults, and override them on the bundle level.

Makes sense but I'd leave that for a follow-up. This patch is already doing too much ;)
Btw I want also the other settings (fallback, ...) to be per-bundle.

Are these changes required for the language selector? What's their effect on the permissions tests in #1770750: Improve test coverage?.

Well, yes, now we can assign LANGUAGE_NONE to any entity type so we have to account for that when deciding whether displaying the translate tab.

These changes make me wonder whether it would be good for developers to attach all ET form handlers in one place... so maybe we should also move the #submit handler into hook_form_alter()? Not sure.

Well, the validation handler is a hack and is really hardcoded logic, while the submission handlers should be overridable, if needed, by the specialized translation handlers. I'd leave this as is.

Maybe we should also add a respective comment to the handler interface docs, saying that the entity might only be a stub entity.

Not sure where, honestly.

plach’s picture

I want to merge my topic branch so please don't commit any patch here.

plach’s picture

Last one: fixed a bug introduced by #11 and adds an option to exclude LANGUAGE_NONE from the available languages.

plach’s picture

Just posted a new patch in #1495570: Update Entity translation integration which relies on this. It would be good to commit this and then get it RTBC ASAP, since the new Commerce release should be out on October 5th. Testing the patch spawned a couple of new issues:

#1798456: Hide shared form elements when the user has not the related permission
#1798460: Make the translate operation contextual

bforchhammer’s picture

Status: Needs review » Reviewed & tested by the community

Awesome! Patch looks good to me.

Minor documentation issues:

+++ b/includes/translation.handler.inc
@@ -837,6 +907,12 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
+    // might render impossibile to make this check after form submission we

"Impossible" has an "i" too much.

+++ b/entity_translation.module
@@ -9,6 +9,25 @@ module_load_include('inc', 'entity_translation', 'entity_translation.node');
+/**
+ *
+ * @var unknown_type
+ */
+define('ENTITY_TRANSLATION_LANGUAGE_CURRENT', 'xx-et-current');

I think these doc blocks can be removed. ;)

plach’s picture

Status: Reviewed & tested by the community » Fixed

Fixed #15, committed and pushed, thanks!

Now we should really focus on getting #1495570: Update Entity translation integration RTBC :)

plach’s picture

Issue tags: +Needs tests

This will need test coverage.

bforchhammer’s picture

Follow-up issue for bean module: #1799776: Fix entity translation regressions

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

  • Commit a88d15e on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions by plach:
    Issue #1280546 by plach: Introduced a language selection widget for...
  • Commit 84bff89 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions by plach:
    Issue #1280546 by plach: Added missing file.
    

  • Commit a88d15e on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions, workbench by plach:
    Issue #1280546 by plach: Introduced a language selection widget for...
  • Commit 84bff89 on 7.x-1.x, et-permissions-1829630, factory, et-fc, revisions, workbench by plach:
    Issue #1280546 by plach: Added missing file.