Problem/Motivation

As per #2407093-3: Refactor ConfigurableLanguageManager::updateLockedLanguageWeights() to save configuration entities rather than raw config .

Proposed resolution

Remove setName() from LanguageInterface. Reimplement filterLanguages() with creating a new default language object for the list.

Remaining tasks

Do it. Review. Commit.

User interface changes

None.

API changes

LanguageInterface will not have setName(). It is not supposed to have that logically.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +Novice
Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015Queue
zealfire’s picture

@Gabor, i was trying to solve this issue,but i have some doubts regarding implementation of filter languages().Since we have to remove setName() so do we need to override default language name as it was done by setName() ? I guess removing "$default->setName($this->t("Site's default language (@lang_name)", array('@lang_name' => $default->getName())));" this line would be fine.
Please tell me.Thanks.

Gábor Hojtsy’s picture

The idea is you would create a new Language() with data appropriate for this display variant of the default language only for the runtime.

sidharthap’s picture

Assigned: Unassigned » sidharthap

I am working on it.

DevElCuy’s picture

DevElCuy’s picture

Issue tags: +SprintWeekend2015Queue

Removed SprintWeekend2015Queue by mistake.

sidharthap’s picture

Assigned: sidharthap » Unassigned
Cogax’s picture

Assigned: Unassigned » Cogax
Cogax’s picture

Status: Active » Needs review
FileSize
1.93 KB

I've made something, but i don't now if it's the solution. If not, please help me to get on the right way. Tanks!

Cogax’s picture

Assigned: Cogax » Unassigned

Status: Needs review » Needs work

The last submitted patch, 10: removed-setName-from-LanguageInterface-2407125-10.patch, failed testing.

Cogax’s picture

Assigned: Unassigned » Cogax

Cogax’s picture

Assigned: Cogax » Unassigned
Status: Needs work » Needs review
FileSize
2.71 KB
806 bytes

I updated the Testcases, so there was a test case for the setName method.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

Applies, tests pass. Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -397,9 +397,9 @@ protected function filterLanguages(array $languages, $flags = LanguageInterface:
-      // Setup a language to have the defaults, but with overridden name.
...
+      // Setup a language to have the defaults
       $default = $this->getDefaultLanguage();
-      $default->setName($this->t("Site's default language (@lang_name)", array('@lang_name' => $default->getName())));

This is important - this allows sites to translate Site's default language. We shouldn't be removing this. I think Gabor was thinking we should be creating new language object here.

Gábor Hojtsy’s picture

Issue tags: +sprint

Indeed, looks good except:

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -397,9 +397,9 @@ protected function filterLanguages(array $languages, $flags = LanguageInterface:
+      // Setup a language to have the defaults
       $default = $this->getDefaultLanguage();
-      $default->setName($this->t("Site's default language (@lang_name)", array('@lang_name' => $default->getName())));

Right, we want the label to be different here in the runtime. We should use new Language() for a language we only use in the runtime.

Cogax’s picture

Thanks a lot, i hope i got it now. If not, please help me, i'ts all new to me :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This clarifies the difference between Language and ConfigurableLanguage and it also makes Language a true value object because it is immutable. Nice.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 574b0eb and pushed to 8.0.x. Thanks!

  • alexpott committed 574b0eb on
    Issue #2407125 by Cogax: LanguageInterface should not support setName
    
alexpott’s picture

Status: Fixed » Needs work

Oops missed something - we should be moving LanguageInterface::setName to ConfigurableLanguageInterface since we are not removing ConfigLanguage::setName() - and we shouldn't be removing that.

  • alexpott committed 9243c13 on 8.0.x
    Revert "Issue #2407125 by Cogax: LanguageInterface should not support...
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
2.1 KB

Yeah sorry I don't know how we missed that... Adding some test coverage also to ConfigurableLanguageUnitTest.

Gábor Hojtsy’s picture

FileSize
5.3 KB

Duh patch did not include the changes in #24. Now does.

The last submitted patch, 24: 2407125-24.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2407125-25.patch, failed testing.

Gábor Hojtsy’s picture

Fails seems to be unrelated in Drupal.php where it gets an entity manager.

Status: Needs work » Needs review

Gábor Hojtsy queued 25: 2407125-25.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2407125-25.patch, failed testing.

Gábor Hojtsy’s picture

Hm, related.

Gábor Hojtsy’s picture

@Cogax: wanna continue with this one?

Cogax’s picture

Assigned: Unassigned » Cogax

Sure, I'll try..

Gábor Hojtsy’s picture

@Cogax: how is it going?

Cogax’s picture

I could need some help. I searched a lot and dived deeper and deeper into Drupal (that's the only good thing on that) but with no result.
The only thing that i think we should made is that we should implement a setUp() Method for the ConfigurableLanguageUnitTest. I've tried to setup there a Language Entity Manager (wich is needed in the label() Method of Entity.php) like it's been done in some other tests. But the Error still exists.

/**
 * {@inheritdoc}
 */
public function setUp() {
  $container = new ContainerBuilder();
  $container->set('entity.manager', $this->getMockBuilder('Drupal\language\ConfigurableLanguageManagerInterface')
    ->getMock());
  \Drupal::setContainer($container);
}

I had also problems to debug. I run the test alwas in my console with

php core/scripts/run-tests.sh --color --verbose --file core/modules/language/tests/src/Unit/ConfigurableLanguageUnitTest.php

but i don't know how to make test outputs like print_r / print / echo / var_dump etc. I've written this outputs always in a textfile wich isn't a good soloution..

Cogax’s picture

Assigned: Cogax » Unassigned
vijaycs85’s picture

As mentioned by @Cogax, we need the container to test setName/getName methods. Added new kernel test case which covers this.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Yay, thanks, just some cleanup:

+++ b/core/modules/language/tests/src/ConfigurableLanguageTest.php
@@ -0,0 +1,40 @@
+ * @coversDefaultClass \Drupal\language\Entity\ConfigurableLanguage
...
+   * @covers ::getName
+   * @covers ::setName

These style comments are not used on simpletest based tests, no?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.19 KB
878 bytes

Thanks for the review @Gábor Hojtsy. Here is an update...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and includes only a very small disruptive change that contrib should not have been relying upon, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 4f776f6 and pushed to 8.0.x. Thanks!

  • alexpott committed 4f776f6 on 8.0.x
    Issue #2407125 by Cogax, vijaycs85, Gábor Hojtsy: LanguageInterface...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all!

Status: Fixed » Closed (fixed)

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