Add an API to set active language

Nice

Thanks for the work on this!

I think the patch does the job; however I also feel that there are other modules besides the Title module that may find this useful.

I took the liberty of rewriting this into a hook that will notify all hook implementations that there is an incoming content form language to take into account.

Following from this hook, the Title module can have a much simpler way to - at least - get over the contradicting URLs problems. The code I am posting got through all the tests of Title, including the latest one posted as a patch here (https://www.drupal.org/node/2267251#comment-11903632).

Hi @czigor,

I find that the root of the issue is that the language negotiation logic for URL suffixes is buried deep inside Entity translation. I think this kind of negotiation logic should be exposed on a system-wide level.

In the meantime, I came up with an alternative, without needing to have this dependency between Entity translation and the Title module. https://www.drupal.org/node/2267251#comment-11917811

If that alternative solves the problem, we could close this one without any code modifications.

Thank you all for opening this and getting the ball rolling!

I've been thinking about #2267251: Do not rely on "content language" in hook_entity_presave() for quite some time (intermittently ;) and I agree with @czigor's conclusion that it's Entity Translation's job to define the active language for field content, since it's exposing the actual translation functionality.

My original idea was to introduce a new EntityTranslationHandlerInterface::getActiveLanguage() method (with a corresponding setter) to replace the existing ::getFormLanguage(). This would be the language to be used by entity_translation_language() (the language callback). EntityTranslationHandlerInterface::getFormLanguage() (and the corresponding setter) would be deprecated and kept around for BC-only. They would just be thin wrappers around the new methods.

This approach would allow us to explicitly set the active language through a centralized API that can be reliably used when programmatically dealing with field translations. In Title we would agree on a sensible default behavior and modules implementing use cases for which the default does not work could set the active language explicitly.

Thoughts?

@czigor:

The reason to replace the actual implementation of ::get/setFormLanguage() is that active language is a broader concept that encompasses form language. Basically the entity active language is the form language when dealing with an entity translation form.

I'm talking about active language in these terms because it's a concept "backported" from D8. I introduced it in Title way after the form language concept was added to Entity Translation and that's the reason why in D7 it looks like a Title-only concept. However it's a general concept that architecturally probably belongs to core but, since there's no concept of whole entity translation in D7 core (just translatable fields), ET looks like the best candidate to me.

The Entity module also provides a similar concept with the entity metadata wrapper language, and there's probably quite some overlap it would be worth investigate. Unfortunately when ET was first drafted the Entity module only provided CRUD operations and so its field language handling and ET's diverged quite a bit. It would probably would be a sane idea to rebuild ET 2.0 on top of the Entity API module, but I'm not sure that will ever happen :)

I hope this helps to clarify my position.

I disagree: so far (hopefully) ::set/getFormLanguage() have (or should have) been used almost as an internal API, while ::set/getActiveLanguage() is meant to be a public API everyone dealing with Entity Translations in a programmatic context is expected to use. I agree we probably need to be very clear about this distinction in the PHP docs and perhaps immediately deprecating ::set/getFormLanguage() is not a good idea, as we may discover use cases where their behavior may diverge, although I cannot come up with any concrete example right now.

What if we just add ::set/getActiveLanguage() and change ::set/getFormLanguage() to proxy the call to those for now. If things appear to work smoothly we can then deprecate the form language methods later.

I will work on this ASAP if we agree on the ay forward.

Thanks, I will start from there later today!

And here it is.

I decided to keep form language accessors deprecated as deciding when using those or the new ones would probably add confusion. Right now the instructions are easy: just use the new ones instead of the old ones.

I don't know that part yet but I fully agree with the patch.

Small PHP doc adjustment: the patch is ok, as the test failures were already there in HEAD.

1. +++ b/includes/translation.handler.inc
   @@ -99,6 +99,63 @@ interface EntityTranslationHandlerInterface {
   +   * This is the language that determines which translation for should be
   

   typo

2. +++ b/includes/translation.handler.inc
   @@ -804,6 +851,35 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
   +    // For new entities the form should be built in the default language. The
   

   More form terminology.

3. +++ b/includes/translation.handler.inc
   @@ -1109,32 +1185,19 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
        $args = func_get_args();
   

   mmh, not thin enough

Code looks good, tests are all green.
It also allows the latest patch from #2267251-56: Do not rely on "content language" in hook_entity_presave() to turn the Title tests green too.

Thanks a lot for this.

@czigor:

Do we have your +1? :)

@czigor:

I don't think making it thin will break BC: if you call ::setFormLanguage() it will proxy to ::setActiveLanguage(), which in turn will call ::setActiveLanguage() on the children, which should be the same of calling ::setFormLanguage() on each of them. The point of the new test was exactly to ensure this behavior is preserved. What am I missing?

Mmh, perhaps you were thinking of modules overriding ::setFormLanguage()? That could indeed introduce some BC problems, what if we reversed the internal implementation and make ::get/setActiveLanguage() thin wrappers around ::get/setFormLanguage()? We can go back to the implementation provided by #26 when removing the deprecated functions.

Sorry, crosspost. I checked all the modules I know of that provide translation handlers and none overrides ::setFormLanguage(), OTOH that's not nearly enough to claim we're performing a BC change, so I'd be inclined to go with #33. Thoughts?

Tests are still green. I guess it's either #26 or this.

#35 would break BC the same way as #26 just not now but when we remove the deprecated functions.

Yep, but at that point people would be expected to have transitioned to ::get/setActiveLanguage(), so that will be an expected (and documented) BC break.

It still seems to me that the cleanest (and easiest) solution is not to introduce get/setActiveLanguage() but restate the purpose of get/setFormLanguage() from being an inner API function to a public one.

Easiest for sure. Cleanest, I don't agree, sorry :)

I would not want (or think it is right) to use an API that mentions form language when there is no form involved. ET is still in beta after all, so I think it's ok to perform this kind of changes.

But I'm still fine with going with #26/#35 as well :).

Ok, so back to RTBC. I will leave this around for a few more hours before committing it, to give people time to chime in.

+++ b/includes/translation.handler.inc
@@ -99,6 +99,62 @@ interface EntityTranslationHandlerInterface {
+   * @deprecated in 7.x-1.0-beta6, will be removed before 7.x-1.0. Use set
...
+   * @deprecated in 7.x-1.0-beta6, will be removed before 7.x-1.0. Use set

Typos to be fixed on commit :)

Committed and pushed, thank you all for your help and feedback!

Now it would be good open issues to update the affected modules providing ET integration. I found the following:

• Media
• ECK
• Field Collection

It would also be good to add a change record. I will do it tomorrow if none beats me to it.

Thanks!

And here is the change record:

https://www.drupal.org/node/2855208

I'd like to wait a couple of days before publishing a new release to make sure this did not introduce some unforeseen issue.

There is a patch in Inline Entity Form's issue queue that will probably be affected by this commit, I added a comment in https://www.drupal.org/node/1545896#comment-11953198

Same thing with Paragraphs, I see that Pol knows about this issue:
https://www.drupal.org/node/2452675#comment-11953202

I think Entity Reference also has some kind of Entity Translation integration; I'll leave it to you guys to choose whether to create an issue.

Updated the change record, thanks.

Any time-line for a tagged release? This new api function makes sense.

beta6 is out:

https://www.drupal.org/project/entity_translation/releases/7.x-1.0-beta6

Thanks @plach and everyone else for this.

I've written a draft hook_requirements for the other modules that wish to go ahead safely with this change.

/**
 * If entity_translation is used, make sure it is beta6 or newer.
 */
function some_cool_module_requirements($phase) {
  $requirements = array();
  // Ensure translations don't break during installation.
  $t = get_t();
  if (module_exists('entity_translation')) {
    if ($phase == 'update' || $phase == 'install' || $phase == 'runtime') {
      $entity_translation_info = system_get_info('module', 'entity_translation');
      $et_installed_version = $entity_translation_info['version'];
      $et_installed_datestamp = $entity_translation_info['datestamp'];
      $march3rd_entity_translation_timestamp = 1488530885;
    
      if ($et_installed_datestamp < $march3rd_entity_translation_timestamp) {
        $requirements['entity_translation']['description'] = $t('Your entity_translation installation version: %version is too old. some_cool_module requires entity_translation at least beta6 from march 3 2017 or newer.', array('%version' => $et_installed_version));
        $requirements['entity_translation']['severity'] = REQUIREMENT_ERROR;
        $requirements['entity_translation']['value'] = $et_installed_version;
        $requirements['entity_translation']['title'] = $t('Entity translation (when installed) with some_cool_module');
      }
    }
  }
  return $requirements;
}

This change caused a regression which is fixed by patch 4 in the following issue:

#2863794: Translated content is lost when cloning

media 7.x-2.0 was released Thursday and it now requires beta6, needs related issue fixed, please action this related issue asap.

@joseph.olstad: Thanks for the last 2 comments, this is not the right issue/commit though. The code cited in #2863794: Translated content is lost when cloning originates from #2438615: New nodes are always created using LANGUAGE_NONE (only changed to the correct language during form submission).