Problem/Motivation

We have a pattern where we get a Language from languageManager() and then later call getLanguageConfigOverride(). But getLanguageConfigOverride() is only for ConfigurableLanguage (managers).

for example in
ConfigLanguageOverrideWebTest::testSiteNameTranslation()

   \Drupal::languageManager()
      ->getLanguageConfigOverride($langcode, 'system.site')
      ->set('name', 'XX site name')
      ->save();

(note maybe also added in #1879930: Language selectors are not showing localized to the page language depending on how that goes in.)

Proposed resolution

use a configurable language manager when we need config things.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

No.

API changes

No.

Files: 
CommentFileSizeAuthor
#35 use_get_a-2355665-35.patch14.78 KBManuel Garcia
#28 interdiff-2355665-21-28.txt907 bytescheops90
#28 use-configurable-language-manager-2355665-28.patch14.46 KBcheops90
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,482 pass(es). View
#21 interdiff-2355665-16-21.txt3.47 KBcheops90
#21 use-configurable-language-manager-2355665-21.patch14.26 KBcheops90
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,484 pass(es), 1 fail(s), and 0 exception(s). View
#16 interdiff-2355665-13-16.txt6.62 KBcheops90
#16 use-configurable-language-manager-2355665-16.patch14.1 KBcheops90
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,480 pass(es), 1 fail(s), and 0 exception(s). View
#13 interdiff-2355665-9-13.txt2.35 KBcheops90
#13 interdiff-2355665-3-9.txt8.04 KBcheops90
#13 use-configurable-language-manager-2355665-13.patch12.24 KBcheops90
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,460 pass(es). View
#9 use-Configurable-language-manager-2355665-9.patch12.59 KBcheops90
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,436 pass(es). View
#8 use-Configurable-language-manager-2355665-7.patch12.59 KBcheops90
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,433 pass(es). View
#3 use-Configurable-Language-manager-2355665-3.patch12.28 KBcheops90
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,447 pass(es). View

Comments

YesCT’s picture

Title: use/get a ConfigurableLanguageManager later needing to call getLanguageConfigOverride() » use/get a ConfigurableLanguageManager when later needing to call getLanguageConfigOverride()
cheops90’s picture

Assigned: Unassigned » cheops90
cheops90’s picture

FileSize
12.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,447 pass(es). View
cheops90’s picture

Assigned: cheops90 » Unassigned
Status: Active » Needs review
Xano’s picture

+++ b/core/modules/config/src/Tests/ConfigLanguageOverrideTest.php
@@ -94,32 +94,41 @@ function testConfigLanguageOverride() {
+  /**
+   * Returns the configurable language manager.
+   *
+   * @return \Drupal\language\ConfigurableLanguageManager
+   */
+  protected function languageManager() {
+    return $this->container->get('language_manager');
+  }

Is this done just for type hinting? If so, try doing the following instead. Most IDEs can parse this.

/** @var \Drupal\language\ConfigurableLanguageManager $language_manager */
$language_manager = $this->container->get('language_manager);

cheops90’s picture

Assigned: Unassigned » cheops90

Working on it: Change the type hinting.

YesCT’s picture

Just noting that all instances of this are in tests.

cheops90’s picture

FileSize
12.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,433 pass(es). View
cheops90’s picture

FileSize
12.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,436 pass(es). View

Sorry the comment id was wrong.

cheops90’s picture

Assigned: cheops90 » Unassigned
YesCT’s picture

It is common, and helps reviewers, if when posting a new patch on an issue (that already has a patch) to include an interdiff. For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff

YesCT’s picture

  1. +++ b/core/modules/config/src/Tests/ConfigLanguageOverrideTest.php
    @@ -8,14 +8,14 @@
    -use Drupal\simpletest\DrupalUnitTestBase;
    +use Drupal\simpletest\KernelTestBase;
    

    changes like this seem out of scope for this issue...

    I think #2351251: Replace extend of deprecated DrupalUnitTestBase with KernelTestBase in Config covers that.

  2. +++ b/core/modules/config/src/Tests/ConfigLanguageOverrideTest.php
    @@ -27,6 +27,9 @@ class ConfigLanguageOverrideTest extends DrupalUnitTestBase {
    +    $language_manager = $this->container->get('language_manager');
    +    $this->languageManager = $language_manager;
    

    is languageManager already a property we can set?

    if not, we should probably declare it.

cheops90’s picture

FileSize
12.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,460 pass(es). View
8.04 KB
2.35 KB

Patch from #7 and #9 are exactly the same - only the name is different.

Xano’s picture

That's better! Almost there :)

+++ b/core/modules/config/src/Tests/ConfigLanguageOverrideTest.php
@@ -27,6 +27,9 @@
+    /* @var \Drupal\language\ConfigurableLanguageManager $language_manager */
+    $language_manager = $this->container->get('language_manager');

This is useless, because you are assigning the language manager to a local variable without using it. The type hint should be done where you are calling methods on the object.

Are you using an IDE like PhpStorm?

YesCT’s picture

Thanks!

Dont worry if the patch number gets off by 1 or so from the comment number. that happens sometimes. :)

looks like the out of scope changes are reverted. thanks.

+++ b/core/modules/config/src/Tests/ConfigLanguageOverrideWebTest.php
@@ -41,7 +41,7 @@ function testSiteNameTranslation() {
-    \Drupal::languageManager()
+    $this->container->get('language_manager')

why like this instead of $this->languageManager like the others?

cheops90’s picture

FileSize
14.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,480 pass(es), 1 fail(s), and 0 exception(s). View
6.62 KB

@Xano: You are right this is useless, I removed it. Yes i use PhpStorm.

@YesCT: I used $this->container->get('language_manager') because we need only once languageManager in this class, but for the future improvements i think it is better to have a property, so i changed this too.

Thanks.

Status: Needs review » Needs work

The last submitted patch, 16: use-configurable-language-manager-2355665-16.patch, failed testing.

Xano’s picture

Yes i use PhpStorm.

Cool! Then if you write something like the following code, then right click (in PhpStorm) on getLanguageConfigOverrideStorage and select Go to > Declaration, PhpStorm will show you the method's original declaration in the interface, all thanks to the @var type hint. The other way around works too: if you don't type hint, PhpStorm will think you are calling a non-existing method and will warn you about it.

/** @var \Drupal\language\ConfigurableLanguageManagerInterface $language_manager */
$language_manager = $this->container->get('language_manager);
$language_manager->getLanguageConfigOverrideStorage('en');

As you can see, we should have type hinted on ConfigurableLanguageManagerInterface instead of ConfigurableLanguageManager (always type hint on the interface if there is one). Sorry that I put you on the wrong track!

The last submitted patch, 16: use-configurable-language-manager-2355665-16.patch, failed testing.

cheops90’s picture

FileSize
14.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,484 pass(es), 1 fail(s), and 0 exception(s). View
3.47 KB

Changes ConfigurableLanguageManager with ConfigurableLanguageManagerInterface.

cheops90’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: use-configurable-language-manager-2355665-21.patch, failed testing.

The last submitted patch, 21: use-configurable-language-manager-2355665-21.patch, failed testing.

YesCT’s picture

+++ b/core/modules/locale/src/Tests/LocaleConfigTranslationTest.php
@@ -175,7 +184,7 @@ public function testConfigTranslation() {
-    $override = \Drupal::languageManager()->getLanguageConfigOverride('xx', 'image.style.medium');
+    $override = $this->languageManager->getLanguageConfigOverride('xx', 'image.style.medium');

maybe we cannot reuse the one in the property and need to
$this->languageManager = $this->container->get('language_manager');
again. (with a comment explaining why)

(or not use a property, and just local typehint it.)

cheops90’s picture

FileSize
14.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,482 pass(es). View
907 bytes
cheops90’s picture

Status: Needs work » Needs review
YesCT’s picture

nice. it passes.

I feel like we need someone to *think* about the approach of having the properties.

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.

quietone’s picture

Issue tags: +Needs reroll

Patch no longer applies cleanly.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Rerolling...

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
FileSize
14.78 KB

Manually fixed conflicts in:

  • core/tests/Drupal/KernelTests/Core/Config/ConfigLanguageOverrideTest.php
  • core/modules/config/src/Tests/ConfigLanguageOverrideWebTest.php
  • core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
  • core/modules/locale/src/Tests/LocaleConfigTranslationTest.php
Manuel Garcia’s picture

Issue tags: -Needs reroll

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.