Contrib.social

Fix LocaleConfigSubscriberTest::assertNoConfigOverride method

Open on Drupal.org →
Created on 9 April 2020, over 5 years ago
Updated 24 July 2023, over 2 years ago

Problem/Motivation

There are two issues with \Drupal\Tests\locale\Kernel\LocaleConfigSubscriberTest::assertNoConfigOverride method:

1) Wrong arguments are passed to assertNoConfigOverride() method in multiple places in the test.
The method signature is the following:
protected function assertNoConfigOverride($config_name, $langcode)

The calls are the following:
$this->assertNoConfigOverride($config_name, $key, $source_value, $langcode);

This means the passed $key is accepted as $langcode and the other two are ignored.

2) The assertions, responsible for configuration override validation is not correct:

protected function assertNoConfigOverride($config_name, $langcode) {
    $config_langcode = $this->configFactory->getEditable($config_name)->get('langcode');
    $override = $this->languageManager->getLanguageConfigOverride($langcode, $config_name);
    $this->assertNotEquals($langcode, $config_langcode);
    $this->assertTrue($override->isNew());
}

If the active configuration langcode ($config_langcode) is equal to given passed $langcode, the following assertion will fail even if language override doesn't exist:
$this->assertNotEquals($langcode, $config_langcode);

This assertion was passing before because given $langocde was not a language, it was the configuration key ("test" in our case).

Proposed resolution

Remove useless parameters and remove incorrect assertion from assertNoConfigOverride, it should be enough to check whether config override object is new.

Remaining tasks

Review, commit.

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Locale  →

Last updated 4 months ago

Created by

🇸🇮Slovenia primsi

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment almost 3 years ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

  • Status changed to Needs review over 2 years ago
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & MySQL 8
    last update over 2 years ago
    29,840 pass
  • Comment over 2 years ago →
    🇨🇭Switzerland berdir Switzerland

    Rerolled, going back to a patch for now as I couldn't update the merge request.

    Quite a few conflicts, because parameters were removed without fixing the underlying issue and added return types.

  • Status changed to RTBC over 2 years ago
  • Comment over 2 years ago →
    🇺🇸United States smustgrave

    Refactor looks good and didn't seem to break anything.

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & MySQL 8
    last update over 2 years ago
    29,878 pass
  • Comment over 2 years ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Updating credits

  • Status changed to Needs work over 2 years ago
  • Comment over 2 years ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
    +++ b/core/modules/locale/tests/src/Kernel/LocaleConfigSubscriberTest.php
@@ -389,10 +379,8 @@ protected function deleteLocaleTranslationData($config_name, $key, $source_value
-  protected function assertNoConfigOverride(string $config_name, string $langcode): void {
...
+  protected function assertNoConfigOverride($config_name, $langcode): void {

    Any reason we lost the type-hints here?

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024