Problem/Motivation

0 is a valid array key but the ListElement strips this key which leads to a fatal error and invalid config.

Proposed resolution

Use ltrim() instead of array_filter() in ListElement.

Remaining tasks

User interface changes

API changes

Suggested commit message

git commit -m 'Issue #2395627 by webflo, Leksat: Do not remove 0 from config translation data'

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because Config Translation does not work for sequences with multiple items, field default values, for example.
Issue priority Not critical because Drupal is fully functional even with this bug and it is more a 5% use-case that hits bug
Unfrozen changes Not unfrozen
Prioritized changes This is a prioritized change because it is a bug
Disruption No disruption. (Even modules heavily altering Config Translation's forms will only break under absurd circumstances; instead they will notice that they have one bug less, that they didn't know they had)
Files: 
CommentFileSizeAuthor
#21 2395627-20-v2.patch4.81 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,931 pass(es). View
#20 2395627-20-v2.patch4.81 KBLeksat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,923 pass(es). View
#20 2395627-20.patch5.53 KBLeksat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,870 pass(es). View
#20 2395627-12-20-interdiff.txt1.8 KBLeksat
#12 2395627-12.patch5.34 KBtstoeckler
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,944 pass(es). View

Comments

webflo’s picture

FileSize
3.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,543 pass(es), 5 fail(s), and 0 exception(s). View
5.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,843 pass(es). View
webflo’s picture

Status: Active » Needs review
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2381147: Text and text with summary field default value config does not use the text_format schema type

Awesome, thanks a lot!

I've hit this exact problem over in #2381147-4: Text and text with summary field default value config does not use the text_format schema type. See my 2. over in that comment.

The last submitted patch, 1: config-translation-2395627-test-only.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
  1. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -134,4 +134,21 @@ protected function getGroupTitle(DataDefinitionInterface $definition, array $gro
    +   * Get compound element key.
    

    Docs formatting - should be Gets....

  2. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -134,4 +134,21 @@ protected function getGroupTitle(DataDefinitionInterface $definition, array $gro
    +   * @param $base_key
    +   *   The base key.
    +   *
    +   * @param $key
    

    Unneeded blank(ish) line.

  3. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -134,4 +134,21 @@ protected function getGroupTitle(DataDefinitionInterface $definition, array $gro
    +   * @return string
    +   */
    

    Need to doc what the return value is.

  4. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -134,4 +134,21 @@ protected function getGroupTitle(DataDefinitionInterface $definition, array $gro
    +    return implode('.', array_filter(array($base_key, $key), function ($value) {
    +      return isset($value);
    +    }));
    

    An inline comment might be nice.

  5. This issue is a normal bug so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
tstoeckler’s picture

Just marked #2413481: Sequence translation as duplicate. That one simply uses an ltrim() instead of the array_filter(), which seems a little less (conceptual, not performance) overhead IMO. In any case, that means we should really fix this as multiple people (myself included) have been bitten by this.

tstoeckler’s picture

tstoeckler’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.05 KB
5.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,923 pass(es), 1 fail(s), and 2 exception(s). View

Here we go.

Fixed #5 added a beta evaluation and a suggested commit message, because I brought the ltrim() over from the referenced issue.

tstoeckler’s picture

tstoeckler’s picture

Issue summary: View changes

Well, actually pasting the suggested commit message helps...

Status: Needs review » Needs work

The last submitted patch, 8: 2395627-8.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
5.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,944 pass(es). View

Hehe, 8.0.x changed a bit in the meantime, so the test code itself "failed"...

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint
Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -55,7 +55,7 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
           // Make the specific element key, "$base_key.$key".
    

    This comment may not be accurate anymore. Probably not needed as its a method now.

  2. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -134,4 +134,20 @@ protected function getGroupTitle(DataDefinitionInterface $definition, array $gro
    +   *   The base key or NULL of this is the top-level element.
    

    of => if

    In general it would be great to explain what are base and subkeys now that this is removed from its original surroundings.

  3. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
    @@ -702,6 +702,43 @@ public function testAlterInfo() {
    +  public function testSequenceTranslation() {
    

    Add comment block on method.

Gábor Hojtsy’s picture

Issue tags: -sprint

Not being actively worked on.

Schnitzel’s picture

as @tstoeckler suggested here #2413481: Sequence translation there is a similar approach that would fix that as well, maybe we want to merge these two

Gábor Hojtsy’s picture

@Schnitzel: wanna work on this? :)

tstoeckler’s picture

@Schnitzel: I the latest patch already "merges" the two approaches in that I brought over the ltrim().

Schnitzel’s picture

Assigned: Unassigned » Schnitzel

@Gabor
Wanna have it fixed yes :D
And yep, will check that I or one of my people will spend some time on it.

Leksat’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
5.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,870 pass(es). View
4.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,923 pass(es). View

2395627-20.patch includes all fixes for #14

2395627-20-v2.patch is just another approach. It doesn't add the getElementKey() method, reasons:
- the $base_key variable is already described in getTranslationBuild() and setConfig() methods; there is no need to describe it in another one place
- when I see $element_key = isset($base_key) ? "$base_key.$key" : $key; I don't need any comments to understand what's happennig

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint
FileSize
4.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,931 pass(es). View

Yeah v2 makes more sense, we don't need a method introduced to concatenate strings. Uploading again to make sure this one will be committed and not the other one.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1ff477b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 1ff477b on 8.0.x
    Issue #2395627 by tstoeckler, Leksat, webflo, Gábor Hojtsy: Do not...
Gábor Hojtsy’s picture

Assigned: Schnitzel » Unassigned
Issue tags: -sprint

Yay, thanks all!

Status: Fixed » Closed (fixed)

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