Problem/Motivation

Language module alters configuration during module installation (code>language_modules_installed()). Language module shouldn't touch existing config if the modules are installed during config sync.

Proposed resolution

If we are syncing config, we need to assume the config is right and don't perform any "purging".

Remaining tasks

Patch with tests.
Test should install a profile with language types configuration (negotiation), and verify it's not altered.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#39 2613222-39.patch5.85 KBalexpott
#39 37-39-interdiff.txt1.76 KBalexpott
#37 2613222-37.patch4.58 KB_utsavsharma
#37 interdiff_35-37.txt1.01 KB_utsavsharma
#33 interdiff_28_33.txt3.1 KBameymudras
#33 2613222-33.patch4.46 KBameymudras
#32 2613222-32.patch917 bytesJanvi Dasani
#28 2613222-28.patch4.39 KBwebflo
#26 2613222-26.patch3.49 KBwebflo
#24 2613222-24.test-only.patch3.49 KBwebflo
#24 2613222-24.patch4.38 KBwebflo
#23 2613222-23.patch4.52 KBwebflo
#22 2613222-22.test-only.patch3.62 KBwebflo
#20 interdiff_19-20.txt495 bytespooja saraah
#20 2613222-20.patch4.76 KBpooja saraah
#19 2613222-language-types-sync-19.only-test.patch4.81 KBpenyaskito
#13 2613222-13.patch906 bytesalexpott
#4 2613222-3.patch601 byteswebflo
#2 2613222-2.patch581 byteswebflo

Issue fork drupal-2613222

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo created an issue. See original summary.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Active » Needs review
FileSize
2613222-2.patch581 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 2613222-2.patch, failed testing.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review
FileSize
2613222-3.patch601 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: +Needs tests

This makes sense.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer commented
Status: Needs review » Needs work

Per #5

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Version: 8.6.x-dev » 8.8.x-dev
Status: Needs work » Needs review
FileSize
2613222-13.patch906 bytes

Another variant of this has been surfaced by #3061489: Umami changes the admin interface language based on the current page - we also have the same problem if the profile overrides the language.types configuration.

shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
CreditAttribution: shaal as a volunteer and at Palantir.net for Tableau commented

Thank you, applying this patch resolved that other Umami patch!

Screenshot:

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Version: 9.4.x-dev » 9.5.x-dev
FileSize
2613222-language-types-sync-19.only-test.patch4.81 KB

I'm working on this during DrupalCon Prague sprints.

Only-test patch. I would expect this one to fail, but somehow is passing. I need to understand better how the language types purging works for triggering the error.

pooja saraah’s picture

pooja saraah CreditAttribution: pooja saraah at Material for Drupal India Association commented
FileSize
2613222-20.patch4.76 KB
interdiff_19-20.txt495 bytes

Fixed failed commands on #19
Attached patch against Drupal 9.5.x

penyaskito’s picture

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

Tagging this with DrupalCon Prague sprints tag.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2613222-22.test-only.patch3.62 KB

The issue is related to the installer, not config import directly. Here is a failing test.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2613222-23.patch4.52 KB

Patch with test and the fix.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2613222-24.patch4.38 KB
2613222-24.test-only.patch3.49 KB

Fixed CS.

Status: Needs review » Needs work

The last submitted patch, 24: 2613222-24.test-only.patch, failed testing. View results

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review
FileSize
2613222-26.patch3.49 KB 
+++ b/core/modules/language/tests/src/Functional/LanguageInstallerConfigTest.php
@@ -0,0 +1,42 @@
+namespace Drupal\Tests\language\Kernel;

The namespace is wrong.

Status: Needs review » Needs work

The last submitted patch, 26: 2613222-26.patch, failed testing. View results

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review
FileSize
2613222-28.patch4.39 KB

#26 was a test-only patch. Now everything together ...

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Issue summary: View changes
Issue tags: -Needs tests

Updated issue summary

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/language/tests/src/Functional/LanguageInstallerConfigTest.php
@@ -0,0 +1,42 @@
+class LanguageInstallerConfigTest extends BrowserTestBase {

    We need a better name for this test. 

  2. +++ b/core/modules/language/tests/src/Functional/LanguageInstallerConfigTest.php
@@ -0,0 +1,42 @@
+  /**
+   * Tests the configuration events are not fired during install of overrides.
+   */
+  public function testLanguageConfigInstall() {

    This phpdoc comment need to be adjusted, I copypasted from a different test, my bad.

    We might want to rename the test method too.

    Otherwise this is RTBC :-)

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Janvi Dasani’s picture

Janvi Dasani CreditAttribution: Janvi Dasani commented
FileSize
2613222-32.patch917 bytes

Added patch #28 in 10.1.x

ameymudras’s picture

ameymudras CreditAttribution: ameymudras at Salsa Digital commented
Status: Needs work » Needs review
FileSize
2613222-33.patch4.46 KB
interdiff_28_33.txt3.1 KB

Trying to address #30

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito as a volunteer commented
Status: Needs review » Reviewed & tested by the community

That looks good to me, thanks for rerolling and fixing #30.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/language/language.module
@@ -290,6 +291,10 @@ function language_negotiation_url_prefixes_update() {
 function language_modules_installed($modules) {
+  if (\Drupal::isConfigSyncing() || InstallerKernel::installationAttempted()) {

hook_modules_installed() has a second parameter of function hook_modules_installed($modules, $is_syncing) {

I'm also not sure about the check for during the installer - why are we adding that here? I see that is from Webflo's test. Need to think about this some more. I think this tricky because that is the profile replacing config functionality. I'm not sure what's correct here.

So this should be

if ($is_syncing) {
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Discussed with @penyaskito. We need to check if the profile is shipping its own version and we're during installation. That will allow us to fix the umami bug.

_utsavsharma’s picture

_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
FileSize
interdiff_35-37.txt1.01 KB
2613222-37.patch4.58 KB

Addressed the point from #35.
Please review.

Status: Needs review » Needs work

The last submitted patch, 37: 2613222-37.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
37-39-interdiff.txt1.76 KB
2613222-39.patch5.85 KB

Here's what I mean by #36...

penyaskito’s picture

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

You beat me before I found time to tackle this, thanks Alex!

I've tested #3061489: Umami changes the admin interface language based on the current page now with this patch and it works as expected. Do you think we need anything else for closing this one and de-postpone the other one?

Thanks again!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@penyaskito I don't think so. We just need reviews and a rtbc here.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Reviewing patch #39

Running the test locally just to make sure they fail.

Testing /var/www/html/web/core/modules/language/tests/src/Functional

Failed asserting that false is true.
 /var/www/html/web/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
 /var/www/html/web/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
 /var/www/html/web/core/modules/language/tests/src/Functional/LanguageConfigInstallOverrideExistingTest.php:35
 /var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Rest looks good using is_syncing

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 2613222-39.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott opened merge request !5731

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Hiding all the files

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Reran tests locally because I can't run the test-only feature even after getting push access and still get a failure.

Previously RTBC so restoring status.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

  • catch committed 96420cbd on 10.2.x 
    Issue #2613222 by webflo, alexpott, ameymudras, pooja saraah,...

  • catch committed 0069d077 on 10.3.x 
    Issue #2613222 by webflo, alexpott, ameymudras, pooja saraah,...

  • catch committed 7e2db381 on 11.x 
    Issue #2613222 by webflo, alexpott, ameymudras, pooja saraah,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, cherry-picked to 10.3.x and 10.2.x, thanks!

Status: Fixed » Closed (fixed)

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