This is a followup of #2849464: Add an API to set active language and #2438615: New nodes are always created using LANGUAGE_NONE (only changed to the correct language during form submission).

#2849464: Add an API to set active language created EntityTranslationDefaultHandler::getActiveLanguage() and deprecated EntityTranslationDefaultHandler::getFormLanguage(). To preserve BC, for the time being getActiveLanguage() was implemented as a wrapper around getFormLanguage(). Modules trying to determine the active language should use getActiveLanguage(). That's what the Title module patch #2267251-65: Do not rely on "content language" in hook_entity_presave() is trying to do.

However, just a few weeks before introducing getActiveLanguage(), getFormLanguage() got some form-specific code in #2438615: New nodes are always created using LANGUAGE_NONE (only changed to the correct language during form submission). Since getFormLanguage() is used now in a broader context (on entity display, for example), the form specific code should be removed and formGetLanguage() should default to the current content language instead. As a general rule, getActiveLanguage() should not include logic to determine the language, but the menu router callback or whatever should call setActiveLanguage() instead.

As per #2438615: New nodes are always created using LANGUAGE_NONE (only changed to the correct language during form submission), this solution will set the translatable field's language in the form array to the current content language. I'm not sure what the desired behavior is there, but tests pass.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

czigor created an issue. See original summary.

czigor’s picture

czigor’s picture

Just to make it clear: the bug I'm trying to fix appears when using the title module and can be reproduced by the test added in #2267251-63: Do not rely on "content language" in hook_entity_presave().

plach’s picture

I'm sorry but I don't agree with the proposed change: I didn't review/commit #2438615: New nodes are always created using LANGUAGE_NONE (only changed to the correct language during form submission) myself, but the approach taken there makes total sense to me, especially now that we added the active language concept and thus we're opening ourselves to proper programmatic workflows. AAMOF I don't think the settings determining the default language make sense only in the form context.

In general I learned the hard way that relying on the current language in contexts different from rendering / visualization leads to fragile and error-prone code, so if I had to change this behavior once again I'd go for a reliable default (e.g. the site default language). However, as I said, the current behavior makes sense to me and, if I'm not mistaken, is consistent with what we implemented in D8, thus I'd like to keep things the current way.

plach’s picture

If, for any reason, the configured default does not make sense to you, you now have the option to change it via ::setActiveLanguage() after all.

czigor’s picture

@plach Do you recommend to call setActiveLanguage($current_content_language) on every entity render inside a hook_entity_view()?

If we want (do we?) to use the new API instead of title_active_language() (which #2267251: Do not rely on "content language" in hook_entity_presave() is partly about) I see two options:
1. Use the current implementation of getActiveLanguage() and call setActiveLanguage($current_content_language) on every entity view (maybe inside Title module).
2. Use the patch #2 and call setActiveLanguage() for the edge cases like #2438615: New nodes are always created using LANGUAGE_NONE (only changed to the correct language during form submission).

czigor’s picture

I don't think the settings determining the default language make sense only in the form context.

Can you explain this? The entity default language is the one in which the entity was first created in, right? In what other context than form is it interesting?

stefanos.petrakis@gmail.com’s picture

I read this issue a number of times in the last few days, as well as the current code of getActiveLanguage()/getFormLanguage(). The priority list of system-wide and entity-specific settings goes like this:

  1. $entityhandler->activeLanguage
  2. $entity_translation_settings_ENTITY_TYPE__ENTITY_BUNDLE['default_language']
  3. $entity->language

This priority list reads as a set of guidelines:

a) If you want your EntityTranslationHandler to use a specific language for its operations, set the active language on it (programmatically).
b) Newly created entities will use system-wide variables per entity type and bundle (@see ::getDefaultLanguage()). You can configure this via the admin interface or programmatically.
c) Existing entities without an activeLanguage set on their respective EntityTranslationHandler, will be handled using their set language property which can be set either via the admin interface or programmatically.

I find this already leaves enough space to be in full control of what is happening. So, I would go for option 1 from
comment #6 and set the active language from inside the title module, even inside the title_entity_load(). This could look sth like this:

/**
 * Implements hook_entity_load().
 *
 * Since the result of field_attach_load() is cached, synchronization must be
 * performed also here to ensure that there is always the correct value in the
 * replaced fields.
 */
function title_entity_load($entities, $type) {
  // Load entity translations otherwise field language will not be computed
  // correctly.
  module_invoke('entity_translation', 'entity_load', $entities, $type);
  $active_language = $GLOBALS['language_content']->language;
  foreach ($entities as &$entity) {
    if (module_invoke('entity_translation', 'enabled', $type)) {
      $handler = entity_translation_get_handler($type, $entity);
      $handler->setActiveLanguage($active_language);
    }
    // Synchronize values from the regular field unless we are intializing it.
    title_entity_sync($type, $entity, NULL, !empty($GLOBALS['title_field_replacement_init']));
  }
}

But there would still be a problem there, as demonstrated by the new tests added in #2267251-63: Do not rely on "content language" in hook_entity_presave(), especially the following snippet:

    // Check that legacy fields are properly synchronized on entity load.
    $term = $this->termLoad($term->tid, $translation_langcode);
    foreach ($translated_values as $name => $value) {
      $this->assertEqual($term->{$name}, $value, t('Legacy field "@field" is properly synchronized when term is loaded in translation language.', array('@field' => $name)));
    }

The problem here is that the EntityTranslationHandler has already had an activeLanguage explicitly set when calling termLoad(), which when the entity_load is called, will be overwritten, effectively breaking the test. I have the notion that calling setActiveLanguage() should allow to overwrite the current activeLanguage in a soft or hard way, sth like this:

  /**
   * {@inheritdoc}
   */
  public function setActiveLanguage($langcode, $overwrite = TRUE) {
    // @todo To fully preserve BC, we proxy the call to the deprecated
    //   ::setFormLanguage method. This will keep things working even when it
    //   has been overridden. Inline its implementation here upon removal.
    if (empty($this->activeLanguage) || $overwrite) {
      $this->setFormLanguage($langcode);
    }
  }

That way, the above call to setActiveLanguage() could be rewritten like:

/**
 * Implements hook_entity_load().
 *
 * Since the result of field_attach_load() is cached, synchronization must be
 * performed also here to ensure that there is always the correct value in the
 * replaced fields.
 */
function title_entity_load($entities, $type) {
  // Load entity translations otherwise field language will not be computed
  // correctly.
  module_invoke('entity_translation', 'entity_load', $entities, $type);
  $active_language = $GLOBALS['language_content']->language;
  foreach ($entities as &$entity) {
    if (module_invoke('entity_translation', 'enabled', $type)) {
      $handler = entity_translation_get_handler($type, $entity);
      // Overwrite the activeLanguage property if not already set,
      // with a sensible default.
      $handler->setActiveLanguage($active_language, FALSE);
    }
    // Synchronize values from the regular field unless we are intializing it.
    title_entity_sync($type, $entity, NULL, !empty($GLOBALS['title_field_replacement_init']));
  }
}

This change is BC and will allow - since we are talking about workflows often - an even more flexible way to set the activeLanguage; either overwrite it (default) or keep the existing value.

Here is a patch, that, despite this issue's title, leaves getActiveLanguage() unchanged, but allows setActiveLanguage() to function in a soft or hard overwrite mode. I tested this with the title_entity_load() snippet and the latest patch from #2267251: Do not rely on "content language" in hook_entity_presave() and the tests were all green.

P.S.: Pardon the long write-up. :-)

Pol’s picture

Looks like I posted in the wrong thread, this patch is for the title module.

See the thread: #2267251: Do not rely on "content language" in hook_entity_presave().

Sorry for the noise.

Status: Needs review » Needs work

The last submitted patch, 9: 2856927.patch, failed testing.

stefanos.petrakis@gmail.com’s picture

Status: Needs work » Needs review

Hiding the misplaced patch file and setting back to "Needs review"

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

This change makes sense and excellent backwards compatibility.
actually I think I commented on the wrong issue, I am not actually using this patch

joseph.olstad’s picture

Status: Reviewed & tested by the community » Needs review
Delphine Lepers’s picture

Status: Needs review » Reviewed & tested by the community

I am using this patch and it works fine for me, I am marking it as RTBC

stefanos.petrakis@gmail.com’s picture

Hi @DelphineLepers, were you referring to patch from #8? And did you use it in combination with other patches?

Delphine Lepers’s picture

Hi @setfanos.petrakis
I can confirm that using ET 1.1, patch #8 works for me without the title patch https://www.drupal.org/project/title/issues/2267251

stefanos.petrakis@gmail.com’s picture

Status: Reviewed & tested by the community » Closed (works as designed)

Thanks for the update @DelphineLepers,
Applying the patch from #8 should have no effect, unless the other end rewrites the call to setActiveLanguage() using the overwrite argument (setting it to FALSE).
Do you have a specific case where using the patch solved a problem you were facing?

After re-reading this issue's history again, I am going to tag this with a works-as-designed status. The original suggestion from @czigor has been discussed and counter-argued. At the end of the day, there is no strong arguments for such a major change. And as a spin-off from this discussion, a suggestion (from #8) for extending the API may end up providing all the needed flexibility.
#3099093: setLanguage() dual setter extension

If anyone feels differently, feel free to re-open the issue.