Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The TranslatableInterface::isTranslatable()
method allows to determine whether the object is translatable.
The entity system determines the value returned by its implementation via the bundle info translatable
property.
Content Translation alters it based on its settings, however other implementations acting before it may not know a bundle is translatable and so they cannot react to that.
Proposed resolution
Make sure content content_translation_entity_bundle_info_alter()
is invoked before any other implementation.
Remaining tasks
- Validate the proposed solution
Write a patch- Reviews
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#23 | ct-bundle_info_alter-2939909-23.patch | 4.49 KB | plach |
#23 | ct-bundle_info_alter-2939909-23.test.patch | 3.58 KB | plach |
Comments
Comment #2
plachComment #3
plachComment #4
Wim LeersTest coverage review:
So this stores in the
state
service the value for thetranslatable
property of theentity_test_mul
entity type's only bundle that is observed in the alter hook.This then enables Content Translation for this entity type.
This verifies that the observation stored in
state
is in factTRUE
because the previous point set it.👌
Fix review:
Trivial 👍
Comment #5
Wim LeersBy the way: super clear issue summary!
Comment #8
plachPHPStorm suggested the wrong
KernelTestBase
class to extend...Comment #10
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLooks great. Adding reviewer credit.
Comment #13
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.6.x and 8.5.x.
Comment #16
xjmFor reasons I have not investigated, this introduced a fail in
ConfigImportUITest
in 8.5.x HEAD:https://www.drupal.org/pift-ci-job/871671
The fail only happened on 8.5.x, not 8.6.x. I confirmed that the test fails locally and that it passes when this commit is reverted.
Comment #17
xjmQueued an 8.5.x test for the patch.
Comment #18
xjmYep, it fails; see above.
Comment #19
plachSorry about that, this should fix the test failure on 8.5.x.
Comment #20
plachComment #22
effulgentsia CreditAttribution: effulgentsia at Acquia commented#19 interdiff looks good. The main change is to move the test module code out from a new module ("a_content_translation_test") and into an existing test module ("content_translation_test"). I'm not clear on why the creation of a new test module creates the 8.5.x test failure in #16, but moving the code to "content_translation_test" is cleaner anyway, so +1.
However:
The comment needs to be changed to refer to the correct module name. Also, while it was clearer why "a_content_translation_test" would normally be invoked first (coming earlier in the alphabet than "content_translation"), it's not clear at all why "content_translation_test" would normally come before "content_translation". But sure enough, turns out the failure in the ".test" patch in #19 proves that it does. Turns out the reason is that
content_translation_install()
sets its module weight to 10. We should add that information to this code comment as well as add an assertion thatcontent_translation_test
has a smaller module weight thancontent_translation
, since without that condition being met, this test would pass even without the bugfix.Comment #23
plachGood catch! This should address #22.
Comment #24
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #25
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRTBC if tests pass.
Comment #29
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAs I had already committed this previously, and the only changes since the revert are to tests, I committed this again to 8.6 and 8.5, despite also having been the one to re-RTBC it.