Problem/Motivation

Visiting the config translation listing of fields you get a WSOD

Proposed resolution

Fix +
Add testing for that page.

Remaining tasks

Review patch.

User interface changes

No WSOD.

API changes

None.

Data model changes

None.

Release notes snippet

None.

Original Report

\Drupal\config_translation\Controller\ConfigTranslationFieldListBuilder::createInstance doesn't pass the 'entity_type.bundle.info' service to the constructor in the same class.

That would mean that deprecation cannot be removed in D9.

But it was already removed in D9.0.0, so means that

a) we are lacking coverage for something in the list builder;
b) that argument was not needed at all

CommentFileSizeAuthor
#14 3131388-field-config-translation-ui-test-14.patch2.86 KBpenyaskito
#14 3131388-field-config-translation-ui-test-14.only-tests.patch1.91 KBpenyaskito
#8 3131388-field-config-translation-ui-test-7.patch4.01 KBpenyaskito
#8 3131388-field-config-translation-ui-test-7.only-test.patch3.06 KBpenyaskito
#5 3131388-field-config-translation-ui-test-5.patch1.64 KBpenyaskito
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

penyaskito created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

> b) that argument was not needed at all

It's defined as required, which means it is. a) seems to be the only possible explanation, no test coverage of field translation overview?

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented

Sorry, wrote in a rush. Meant the parameter was unused in executed code paths or untested.
I think it's only used for knowing if the bundle column must be shown, I'm writing a test for that.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Simply instantiating the class should be enough to trigger test fails. Passing (implicit) NULL to an argument that is required should cause php notices/warnings which in turn should cause tests to fail.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Version: 8.9.x-dev » 9.0.x-dev
Status: Active » Needs review
FileSize
3131388-field-config-translation-ui-test-5.patch1.64 KB

This should fail in D9

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented

Error in D9 while visiting a fields listing in config translation is:

ArgumentCountError: Too few arguments to function Drupal\config_translation\Controller\ConfigTranslationFieldListBuilder::__construct(), 3 passed in /Users/penyaskito/Projects/lingod9/web/core/modules/config_translation/src/Controller/ConfigTranslationFieldListBuilder.php on line 58 and exactly 4 expected in Drupal\config_translation\Controller\ConfigTranslationFieldListBuilder->__construct() (line 76 of /Users/penyaskito/Projects/lingod9/web/core/modules/config_translation/src/Controller/ConfigTranslationFieldListBuilder.php)

Status: Needs review » Needs work

The last submitted patch, 5: 3131388-field-config-translation-ui-test-5.patch, failed testing. View results

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Category: Task » Bug report
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
3131388-field-config-translation-ui-test-7.only-test.patch3.06 KB
3131388-field-config-translation-ui-test-7.patch4.01 KB

That test was wrong. No need to add a dependency on block_content module, I rewrote that.

I'm going to split this in two issues, for 9.0.x and 8.9.x, as the output is quite different.

9.0.x is definitely a major bug, as we get a WSOD in a Drupal core page with just visiting it.
8.9.x is, IMHO, a major (task) too, but maybe can be demoted to Normal. In any case I hope it goes into 8.9.x release for deprecation sake.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Title: ConfigTranslationFieldListBuilder::createInstance missing entity_type.bundle.info service » Visiting the config translation listing of fields you get a WSOD
Issue summary: View changes
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented

Created #3131435: ConfigTranslationFieldListBuilder::createInstance missing entity_type.bundle.info service for the 8.9.x issue.

The last submitted patch, 8: 3131388-field-config-translation-ui-test-7.only-test.patch, failed testing. View results

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Needs work

Wow, good find. Thanks for investigating and providing a fix. Patch looks good other than:

+++ b/core/modules/config_translation/tests/src/Functional/ConfigTranslationOverviewTest.php
@@ -75,7 +80,7 @@ protected function setUp(): void {
-  public function testMapperListPage() {
+  public function xxxtestMapperListPage() {

@@ -128,7 +133,7 @@ public function testMapperListPage() {
-  public function testHiddenEntities() {
+  public function xxxtestHiddenEntities() {

@@ -149,7 +154,7 @@ public function testHiddenEntities() {
-  public function testListingPageWithOverrides() {
+  public function xxxtestListingPageWithOverrides() {

Accidental changes.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Also what are the differences between this and #3131435: ConfigTranslationFieldListBuilder::createInstance missing entity_type.bundle.info service? It should be possible to post on one issue (select "Test with" when uploading the patch) both patches. I don't see what's the difference sorry.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
FileSize
3131388-field-config-translation-ui-test-14.only-tests.patch1.91 KB
3131388-field-config-translation-ui-test-14.patch2.86 KB

Let's try that then and I'll close the other one.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Needs work » Needs review

The last submitted patch, 14: 3131388-field-config-translation-ui-test-14.only-tests.patch, failed testing. View results

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 9.0.x-dev » 8.9.x-dev
Priority: Major » Critical
Status: Needs review » Reviewed & tested by the community

While this is only a WSOD in Drupal 9, raising to critical because we are supposed to file issues against 8.9 first.

This looks good to me now. I only wonder how far back should we add this, given that it is "only" a deprecation in Drupal 8.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented

A reason to commit this to 8.9: any contrib tests that may hit the config translation fields overview pages are going to fail if not marked as legacy. As an example, see #3129752: Remove @group legacy from our tests.

  • catch committed d44afee on 9.1.x 
    Issue #3131388 by penyaskito: Visiting the config translation listing of...

  • catch committed 82f4469 on 9.0.x 
    Issue #3131388 by penyaskito: Visiting the config translation listing of...

  • catch committed 0954192 on 8.9.x 
    Issue #3131388 by penyaskito: Visiting the config translation listing of...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!

Status: Fixed » Closed (fixed)

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