Request
By utilizing either entity_translation_enabled_bundle() or entity_translation_enabled()
we can make the function entity_translation_language() react more specifically/strictly when called within the context of a specific entity bundle. At the moment, it is a callback that is used for all entity bundles of an entity and it seems that there exist a number of system where some of the bundles of the same entity will not be configured to use ET. In such cases it would be good to detect this early on and avoid the subsequent calls that are either unnecessary or are allowing configurations that are not supported.

Currently
At the moment, this function is not bundle-aware and will - in most cases - delegate further by calling $handler->getActiveLanguage.

Expected outcome
Implementing some bundle-specific filtering logic would:

+ Avoid hiding the logic for determining the activeLanguage of the given entity in other classes/objects
+ Allow immediately returning an appropriate default for entities that are not supposed to have ET handling in the first place.
+ Avoid unsupported integration, e.g. between ContentTranslation and ET

Affected issues
+#2870524: Field copy fails with content translation for fields with entity translation enabled
+#2438615: New nodes are always created using LANGUAGE_NONE (only changed to the correct language during form submission)

Expected side-effects
+ This recent commit will become - probably - obsolete [1d5493]
+ The documentation effort should be simplified, especially when trying to provide guidance for all the various integration coctails

CommentFileSizeAuthor
#7 et-bundle_language_with_test-2877074-7.patch2.59 KBjoseph.olstad
#5 entity_translation-fix_content_translation_test-2877074-4.patch1.1 KBstefanos.petrakis@gmail.com
#3 et-bundle_language-2877074-3.patch1.49 KBplach
#2 et-bundle_language-2877074-2.patch1.67 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stefanos.petrakis created an issue. See original summary.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer commented
Status: Active » Needs review
FileSize
et-bundle_language-2877074-2.patch1.67 KB

I don't we can write entity-type agnostic code without instantiating the entity translation handler. What we could do, is generalizing the logic that will work for the nodes and falling back to directly retrieving the entity language when the bundle is not ET-enabled (see the attached patch).

Thoughts?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer commented
FileSize
et-bundle_language-2877074-3.patch1.49 KB

Now with less whitespace changes!

Status: Needs review » Needs work

The last submitted patch, 3: et-bundle_language-2877074-3.patch, failed testing.

stefanos.petrakis@gmail.com’s picture

stefanos.petrakis@gmail.com
Primary language English
Location Biel, Switzerland
CreditAttribution: stefanos.petrakis@gmail.com as a volunteer commented
Status: Needs work » Reviewed & tested by the community
FileSize
entity_translation-fix_content_translation_test-2877074-4.patch1.1 KB

I was hoping to somehow be able to avoid having to call $handler->getLanguage();, added a relevant issue #2877620: Allow generic fallback logic for entity_language() callbacks that describes the reasoning; code-duplication basically.

The patch covers what I reported in the OP by using entity_translation_enabled().
It also does solve this issue #2870524-21: Field copy fails with content translation for fields with entity translation enabled without the need for the locale patching from #2870524-34: Field copy fails with content translation for fields with entity translation enabled.

Code looks good, the following (not introduced by this patch) could be fixed on commit:

+++ b/entity_translation.module
@@ -1832,9 +1832,9 @@ function entity_translation_settings($entity_type, $bundle) {
  *    The the type of the entity.

Remove the repeated 'the'.

In order to have the tests running properly, the - newly added - EntityTranslationContentTranslationTestCase would need to have its own createPage() method similar to the TranslationTestCase::createPage() one. The provided patch allows tests to run clean with the new code. And yes, it does duplicate code. The irony of it does not escape me. :-/

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

awesome, we should get this in a release, because beta6 requires too many patches.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
FileSize
et-bundle_language_with_test-2877074-7.patch2.59 KB

I think we need to roll up the test patch #2877074-5: Refactor the entity_translation_language() callback to make it bundle-specific
with the fix

here's the rolled up patch
make sure it passes.

dsutter’s picture

dsutter CreditAttribution: dsutter as a volunteer commented

RTBC+ patch #7

gdaw’s picture

gdaw CreditAttribution: gdaw as a volunteer commented

RTBC ++ looks good

  • stefanos.petrakis committed 2100a4d on 7.x-1.x 
    Issue #2877074 by plach, stefanos.petrakis, joseph.olstad: Refactor the...
stefanos.petrakis@gmail.com’s picture

stefanos.petrakis@gmail.com
Primary language English
Location Biel, Switzerland
CreditAttribution: stefanos.petrakis@gmail.com as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Commited, thanks everyone.

Status: Fixed » Closed (fixed)

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