few arrays in Drupal defined same array keys twice, for example:

$elements['checkbox']['element'] = array(
'#title' => $this->randomName(),  // <<< ~~~~~~~ SAME KEY
'#type' => 'checkbox',
'#required' => TRUE,
'#title' => $this->randomName()  // <<< ~~~~~~~  SAME KEY
);
CommentFileSizeAuthor
#29 duplicate_array_key-1599618-29.patch3.75 KBalbert volkman
#26 duplicate_array_key-1599618-26.patch666 bytesalbert volkman
#26 duplicate_array_key-all-changes-do-not-test.patch4.25 KBalbert volkman
#15 1599618-duplicate_array_key-14.patch4.27 KByurtboy
#14 1599618-duplicate_array_key-14.patch4.27 KByurtboy
#7 1599618-duplicate_array_key-7.patch4.17 KBdroplet
#5 1599618-duplicate_array_key2.patch4.28 KBdroplet
kill_duplcate_array_key.patch15.36 KBdroplet

Comments

crifi’s picture

crifi commented
Component: base system » book.module

This isn't the base system.

droplet’s picture

droplet commented
Component: book.module » other
droplet’s picture

droplet commented
Issue tags: -Quick fix, -Needs backport to D7

kill_duplcate_array_key.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Quick fix, +Needs backport to D7

The last submitted patch, kill_duplcate_array_key.patch, failed testing.

droplet’s picture

droplet commented
Status: Needs work » Needs review
StatusFileSize
new 1599618-duplicate_array_key2.patch4.28 KB
mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

Looks good to me, except maybe for:

+++ b/core/modules/system/lib/system/Tests/Batch/PercentagesUnitTest.phpundefined
@@ -55,9 +55,6 @@ class PercentagesUnitTest extends UnitTestBase {
-      // Regardless of number of digits we're using, 100% should always just be
-      // 100%.

Maybe we should remove the 100 => 3/3 test case instead, since the 200/200 testcase tests specifically for numbers with more digits.

droplet’s picture

droplet commented
StatusFileSize
new 1599618-duplicate_array_key-7.patch4.17 KB

The last submitted patch, 1599618-duplicate_array_key-7.patch, failed testing.

droplet’s picture

droplet commented
Status: Needs work » Needs review
Issue tags: +Quick fix, +Needs backport to D7

#7: 1599618-duplicate_array_key-7.patch queued for re-testing.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Title: Remove duplicate array key » Remove duplicate array keys
Status: Needs review » Reviewed & tested by the community

Looks good to me, nice clean-up!

dries’s picture

dries commented

Patch doesn't seem to apply:

vortex:drupal-head dries$ git apply ../f.patch 
error: core/modules/system/tests/form.test: No such file or directory

Asking for a re-test.

dries’s picture

dries commented
Issue tags: -Quick fix, -Needs backport to D7

#7: 1599618-duplicate_array_key-7.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Quick fix, +Needs backport to D7

The last submitted patch, 1599618-duplicate_array_key-7.patch, failed testing.

yurtboy’s picture

yurtboy commented
StatusFileSize
new 1599618-duplicate_array_key-14.patch4.27 KB

Applied patch but it failed but then I modified it to pass locally

Checking patch core/modules/book/book.module...
Checking patch core/modules/file/file.field.inc...
Checking patch core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php...
Checking patch core/modules/system/lib/Drupal/system/Tests/Batch/PercentagesUnitTest.php...
Checking patch core/modules/system/tests/form.test...
error: core/modules/system/tests/form.test: No such file or directory

After modifications

Checking patch core/modules/book/book.module...
Checking patch core/modules/file/file.field.inc...
Checking patch core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php...
Checking patch core/modules/system/lib/Drupal/system/Tests/Batch/PercentagesUnitTest.php...
Checking patch core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php...
Hunk #1 succeeded at 75 (offset 2 lines).
Applied patch core/modules/book/book.module cleanly.
Applied patch core/modules/file/file.field.inc cleanly.
Applied patch core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php cleanly.
Applied patch core/modules/system/lib/Drupal/system/Tests/Batch/PercentagesUnitTest.php cleanly.
Applied patch core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php cleanly.

It applied cleanly at
HEAD is now at 4bd1231 - Patch #1630108 by cosmicdreams: improper reference to Drupal\Database\Query\AlterableInterface.

yurtboy’s picture

yurtboy commented
Title: Remove duplicate array keys » Remove duplicate array keys (take two)
Status: Needs work » Needs review
StatusFileSize
new 1599618-duplicate_array_key-14.patch4.27 KB

seems I did not need a take two. I just misunderstood that status message after the comment was posted.

droplet’s picture

droplet commented
Title: Remove duplicate array keys (take two) » Remove duplicate array keys
Status: Needs review » Reviewed & tested by the community

#15 okay.

chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs work

Thank you for contributing to core but what is going on here?? I see test changes, a new persistent variable introduced, a CSS class changing... omnibus patch :)

droplet’s picture

droplet commented
Status: Needs work » Needs review

@chx,

look at #0 patch with more diff context :)

eg. 

+++ b/core/modules/book/book.moduleundefined
@@ -540,13 +540,12 @@ function _book_add_form_elements(&$form, &$form_state, Node $node) {
     '#attributes' => array(
-      'class' => array('book-form'),
+      'class' => array('book-outline-form'),
     ),
     '#attached' => array(
...
-    '#attributes' => array('class' => array('book-outline-form')),

last one overrided previous one

same problem to others.

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.phpundefined
@@ -75,7 +75,7 @@ class FormTest extends WebTestBase {
-    $elements['checkbox']['element'] = array('#title' => $this->randomName(), '#type' => 'checkbox', '#required' => TRUE, '#title' => $this->randomName());

2 "#title" here :)

chx’s picture

chx commented
Status: Needs review » Active

Ah so. Thanks.

droplet’s picture

droplet commented

??

so remove it . anything wrong ?

droplet’s picture

droplet commented 
var_dump( array('a' => 'xyz', 'a' => 'xyz')  === array('a' => 'xyz') );

output:

bool(true)

chx’s picture

chx commented
Status: Active » Reviewed & tested by the community

Erm. Sorry, wrong status.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

All of these seem to make sense (once you read the surrounding code) except:

       'en,en-US;q=0.5,fr;q=0.25' => 'en',
-      'en-US,en;q=0.5,fr;q=0.25' => 'en-US',

One is keyed en,en-US, the other en-US,en, so I'm not sure how these are duplicates. Also, the above code notes:

      // In our test case, 'en' has priority over 'en-US'.

Therefore, I don't think removing this is correct; the similar values seem to be intentional. If this is wrong, feel free to re-open.

Committed and pushed the rest of the patch to 8.x. Nice catches! Will need a port to D7 due to PSR-0.

In the future, though, please make meaningful issue summaries to explain to people what a patch is doing. That ":)" cost me 10-15 minutes of not committing other peoples' patches. :(

droplet’s picture

droplet commented
Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs work

There're at different places:

      // Different qvalues.
      'en-US,en;q=0.5,fr;q=0.25' => 'en-US',   // <~~~~~~~~~~~~~~~ THIS ONE
      'fr,en;q=0.5' => 'fr-CA',
      'fr,en;q=0.5,fr-CA;q=0.25' => 'fr',

      // Silly wildcards are also valid.
      '*,fr-CA;q=0.5' => 'en',
      '*,en;q=0.25' => 'fr-CA',
      'en,en-US;q=0.5,fr;q=0.25' => 'en',
      'en-US,en;q=0.5,fr;q=0.25' => 'en-US',  // <~~~~~~~~~~~~~~~ THIS ONE
droplet’s picture

droplet commented
Issue tags: +Novice

tagging Novice,

D8 Patch:
- remove #24 duplicate array key

D7 Patch:
- reroll all changes

Thanks.

albert volkman’s picture

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new duplicate_array_key-all-changes-do-not-test.patch4.25 KB
new duplicate_array_key-1599618-26.patch666 bytes

If we revert 7d41f69046999a2e0b90b24c1a24ecc621a978bd, the second patch will apply. First patch includes only changes requested by @droplet.

droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community

Comment #24 explained why we needed it, patch looks good.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Ok, thanks.

Committed and pushed to 8.x. Back to 7.x.

albert volkman’s picture

albert volkman commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new duplicate_array_key-1599618-29.patch3.75 KB

D7 backport.

droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community

5 same changes. Worked.

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/8bd1079

Status: Fixed » Closed (fixed)

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

chrisjlee’s picture

chrisjlee commented
Issue tags: -Quick fix, -Novice, -Needs backport to D7
chrisjlee’s picture

chrisjlee commented
Issue summary: View changes

add summary