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.
Comments
Comment #2
czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commentedComment #3
czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commentedJust 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().
Comment #4
plachI'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.
Comment #5
plachIf, for any reason, the configured default does not make sense to you, you now have the option to change it via
::setActiveLanguage()
after all.Comment #6
czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commented@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 callsetActiveLanguage($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).
Comment #7
czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commentedCan 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?
Comment #8
stefanos.petrakis@gmail.comI 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:$entityhandler->activeLanguage
$entity_translation_settings_ENTITY_TYPE__ENTITY_BUNDLE['default_language']
$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:
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:
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 callingsetActiveLanguage()
should allow to overwrite the current activeLanguage in a soft or hard way, sth like this:That way, the above call to
setActiveLanguage()
could be rewritten like: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 allowssetActiveLanguage()
to function in a soft or hard overwrite mode. I tested this with thetitle_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. :-)
Comment #9
PolLooks 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.
Comment #11
stefanos.petrakis@gmail.comHiding the misplaced patch file and setting back to "Needs review"
Comment #12
joseph.olstadThis change makes sense and excellent backwards compatibility.actually I think I commented on the wrong issue, I am not actually using this patch
Comment #13
joseph.olstadComment #14
Delphine Lepers CreditAttribution: Delphine Lepers at Arhs for European Commission and European Union Institutions, Agencies and Bodies commentedI am using this patch and it works fine for me, I am marking it as RTBC
Comment #15
stefanos.petrakis@gmail.comHi @DelphineLepers, were you referring to patch from #8? And did you use it in combination with other patches?
Comment #16
Delphine Lepers CreditAttribution: Delphine Lepers for European Commission and European Union Institutions, Agencies and Bodies commentedHi @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
Comment #17
stefanos.petrakis@gmail.comThanks 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 toFALSE
).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.