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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach created an issue. See original summary.

plach’s picture

Title: Ensure that hook_bundle_info_alter() implementations know whether bundles are translated » Ensure that hook_bundle_info_alter() implementations know whether bundles are translatable
Issue summary: View changes
Priority: Normal » Major
Issue tags: +blocker
FileSize
3.51 KB
4.42 KB
plach’s picture

Status: Active » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Test coverage review:

  1. +++ b/core/modules/content_translation/tests/modules/a_content_translation_test/a_content_translation_test.module
    @@ -0,0 +1,13 @@
    +  \Drupal::state()->set('a_content_translation_test.translatable', !empty($bundles['entity_test_mul']['entity_test_mul']['translatable']));
    

    So this stores in the state service the value for the translatable property of the entity_test_mul entity type's only bundle that is observed in the alter hook.

  2. +++ b/core/modules/content_translation/tests/src/Kernel/ContentTranslationEntityBundleInfoTest.php
    @@ -0,0 +1,61 @@
    +    $this->contentTranslationManager->setEnabled('entity_test_mul', 'entity_test_mul', TRUE);
    

    This then enables Content Translation for this entity type.

  3. +++ b/core/modules/content_translation/tests/src/Kernel/ContentTranslationEntityBundleInfoTest.php
    @@ -0,0 +1,61 @@
    +    $this->assertTrue($state->get('a_content_translation_test.translatable'));
    

    This verifies that the observation stored in state is in fact TRUE because the previous point set it.

👌


Fix review:

+++ b/core/modules/content_translation/content_translation.module
@@ -60,6 +60,14 @@ function content_translation_module_implements_alter(&$implementations, $hook) {
+    // Move our hook_entity_bundle_info_alter() implementation to the top of the
+    // list, so that any other hook implementation can rely on bundles being
+    // correctly marked as translatable.
+    case 'entity_bundle_info_alter':
+      $group = $implementations['content_translation'];
+      $implementations = ['content_translation' => $group] + $implementations;

Trivial 👍

Wim Leers’s picture

By the way: super clear issue summary!

The last submitted patch, 2: ct-bundle_info_alter-2939909-2.test.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: ct-bundle_info_alter-2939909-2.patch, failed testing. View results

plach’s picture

PHPStorm suggested the wrong KernelTestBase class to extend...

The last submitted patch, 8: ct-bundle_info_alter-2939909-8.test.patch, failed testing. View results

effulgentsia’s picture

Looks great. Adding reviewer credit.

  • effulgentsia committed c9392bb on 8.6.x
    Issue #2939909 by plach, Wim Leers: Ensure that hook_bundle_info_alter...

  • effulgentsia committed 4003008 on 8.5.x
    Issue #2939909 by plach, Wim Leers: Ensure that hook_bundle_info_alter...
effulgentsia’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Pushed to 8.6.x and 8.5.x.

  • xjm committed 384b0fd on 8.5.x
    Revert "Issue #2939909 by plach, Wim Leers: Ensure that...

  • xjm committed a5e3956 on 8.6.x
    Revert "Issue #2939909 by plach, Wim Leers: Ensure that...
xjm’s picture

Status: Fixed » Needs work

For reasons I have not investigated, this introduced a fail in ConfigImportUITest in 8.5.x HEAD:

Action, Ban, Text and Options modules installed in the correct order.
Failed asserting that Array &0 (
    0 => 'action'
    1 => 'ban'
    2 => 'text'
    3 => 'options'
) is identical to Array &0 (
    0 => 'ban'
    1 => 'text'
    2 => 'action'
    3 => 'options'
)

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.

xjm’s picture

Queued an 8.5.x test for the patch.

xjm’s picture

Yep, it fails; see above.

plach’s picture

plach’s picture

Version: 8.5.x-dev » 8.6.x-dev

The last submitted patch, 19: ct-bundle_info_alter-2939909-19.test.patch, failed testing. View results

effulgentsia’s picture

Status: Needs review » Needs work

#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:

+++ b/core/modules/content_translation/tests/src/Kernel/ContentTranslationEntityBundleInfoTest.php
@@ -0,0 +1,64 @@
+    // Check that, although the "a_content_translation_test" hook implementation
+    // by default would be invoked first, it still has access to the
+    // "translatable" bundle info property.

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 that content_translation_test has a smaller module weight than content_translation, since without that condition being met, this test would pass even without the bugfix.

effulgentsia’s picture

Status: Needs work » Needs review
effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if tests pass.

The last submitted patch, 23: ct-bundle_info_alter-2939909-23.test.patch, failed testing. View results

  • effulgentsia committed bc1fc2f on 8.6.x
    Issue #2939909 by plach, xjm, Wim Leers: Ensure that...

  • effulgentsia committed 82a20b9 on 8.5.x
    Issue #2939909 by plach, xjm, Wim Leers: Ensure that...
effulgentsia’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

As 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.

Status: Fixed » Closed (fixed)

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