Problem/Motivation

1. Install Drupal in German.
2. Add French.
3. Go to the German language, create a View. View is created in German. Yay.
4. Go to the French language, create a View. View is still created in German. Nay.

Proposed resolution

Use the negotiated language when defaulting config entity languages.

Remaining tasks

Discuss. Review.

User interface changes

None.

API changes

Default assumptions about config entity language changes. Will only be an actual change on multilingual sites and there the new behavior makes sense, the old one does not.

CommentFileSizeAuthor
#11 interdiff.txt2.12 KBGábor Hojtsy
#11 2447139-11.patch2.95 KBGábor Hojtsy
#8 interdiff.txt1.62 KBGábor Hojtsy
#8 2447139-8.patch2.5 KBGábor Hojtsy
#5 interdiff.txt781 bytesGábor Hojtsy
#5 2447139-5.patch1.61 KBGábor Hojtsy
#1 2447139.patch863 bytesGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2447139.patch863 bytes
Gábor Hojtsy’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1: 2447139.patch, failed testing.

Gábor Hojtsy’s picture

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

It is not clear to me how to tell from the results which test fails exactly. I tried the tests listed around that fatal locally but neither failed. I don't think the code needs fixing, the test needs fixing to define what is missing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2447139-5.patch1.61 KB
interdiff.txt781 bytes

Discussed clever ways to figure out which test was that with Wim Leers and vijaycs85 and we found \Drupal\Tests\Core\Config\Entity\ConfigEntityStorageTest. That of course now needs to return English for the current language then.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Ah there is the issue!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Looks like we should have a test for this - I'm not sure that the test change actually covers the bug being fixed by the patch.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2447139-8.patch2.5 KB
interdiff.txt1.62 KB

Updated tests to ensure that the language default is applied based on current language.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -Needs tests
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Looks great, but some integration test would be great.

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
@@ -139,8 +139,12 @@ protected function setUp() {
+      ->will($this->returnValue(new Language(array('id' => 'hu'))));

Just a comment: For future uses, you can use ->willReturn(new Language(...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Config entities should be created in the page's language » Config entities should be created in the negotiated language unless otherwise specified
Status: Reviewed & tested by the community » Needs review
FileSize
2447139-11.patch2.95 KB
interdiff.txt2.12 KB

Using willReturn() and more coverage for when you did provide a language. The change is in config entity storage, so I think it makes sense to test it there, that applies the default. This should be more complete coverage now.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Looks alright for me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

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 be30a52 and pushed to 8.0.x. Thanks!

  • alexpott committed be30a52 on 8.0.x 
    Issue #2447139 by Gábor Hojtsy: Config entities should be created in the...
Gábor Hojtsy’s picture

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

Yay, thanks!

Status: Fixed » Closed (fixed)

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