The function array_merge_recursive in module_invoke_all (D7) or ModuleHandlerInterface::invokeAll() reindexes arrays if keys are numeric, see comments at http://us.php.net/manual/en/function.array-merge-recursive.php . This caused me problems in Ubercart and may be a problem with some other modules. Is this intended behaviour or can the indexes be preserved?

CommentFileSizeAuthor
#29 376391-29.patch593 bytespietmarcus
#21 interdiff.txt873 bytessnehi
#21 drupal_core-invokeall-reindex-376391-20.patch823 bytessnehi
#20 drupal_core-invokeall-reindex-376391-20-8.x.patch823 bytesmimran
#18 drupal_core-invokeall-reindex-376391-18-8.x.patch837 bytesmimran
#14 drupal_core-invokeall-reindex-376391-14-8.x.patch832 bytesmimran
#6 drupal_core-invokeall-reindex-376391-6-8.x.patch790 bytesmimran
#5 drupal_core-invokeall-reindex-376391-5-8.x.patch761 bytesmimran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented
Version: 6.9 » 8.x-dev
Component: base system » documentation
Category: bug » task

This is the intended behaviour, I just think that it needs documenting that this is what module_invoke_all does.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue tags: +Needs backport to D6, +Needs backport to D7

Sounds like this should be documented (maybe?) and then backported to D6/7.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: module_invoke_all reindexes arrays » module_invoke_all / ModuleHandlerInterface::invokeAll reindexes arrays
Issue summary: View changes
Issue tags: -Needs backport to D6 +Novice

Looking through old issues... It seems like we still need to fix this documentation. Actually, should be a good Novice project. Updating summary.

mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
Assigned: Unassigned » mimran
mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
FileSize
drupal_core-invokeall-reindex-376391-5-8.x.patch761 bytes

Updated the comments with the descriptions

mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
FileSize
drupal_core-invokeall-reindex-376391-6-8.x.patch790 bytes

Updated the patch

mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
Status: Active » Needs review
mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
Assigned: mimran » Unassigned

Status: Needs review » Needs work

The last submitted patch, 6: drupal_core-invokeall-reindex-376391-6-8.x.patch, failed testing.

The last submitted patch, 6: drupal_core-invokeall-reindex-376391-6-8.x.patch, failed testing.

The last submitted patch, 6: drupal_core-invokeall-reindex-376391-6-8.x.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks for the patch! Ignore the test results, there was some kind of a glitch... The patch needs a bit of improvement though:

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
@@ -237,7 +237,8 @@ public function invoke($module, $hook, array $args = array());
+   *   arrays from their implementations, those are merged into one array recursively.

    This line goes over 80 characters. All documentation needs to be wrapped into lines as close to 80 as possible without going over. 

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
@@ -237,7 +237,8 @@ public function invoke($module, $hook, array $args = array());
+   *   Note: integer keys are lost the same as with array_merge_recursive().

    Hm, not the best wording maybe?

    How about:

    Note: integer keys in arrays will be lost, as the merge is done using array_merge_recursive().

    or something similar?

mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
Assigned: Unassigned » mimran
mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
FileSize
drupal_core-invokeall-reindex-376391-14-8.x.patch832 bytes

@jhodgdon, thanks Updated the patch accordingly

mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
Assigned: mimran » Unassigned
Status: Needs work » Needs review
snehi’s picture

snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented

Looks ok to me +1 for RTBC.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

That (obviously) looks good to me too. Except that it should all be wrapped into one paragraph, with lines as close to 80 characters as possible, without going over. Thanks!

mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
FileSize
drupal_core-invokeall-reindex-376391-18-8.x.patch837 bytes

wrapped into one paragraph

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Please don't make it all into one *sentence* -- it is rather run-on. Just make it into one *paragraph*. Thanks!

mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
FileSize
drupal_core-invokeall-reindex-376391-20-8.x.patch823 bytes

updated accordingly

snehi’s picture

snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented
FileSize
drupal_core-invokeall-reindex-376391-20.patch823 bytes
interdiff.txt873 bytes

uploaded

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

Thanks, that's better!

Reminder to contributors: when you upload a new patch after previous patches, please make an interdiff file. Also set the status of issue to "Needs review". (No need to either now.)

Anyway, the patch in #21 is slightly better wrapped, so let's use that.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: module_invoke_all / ModuleHandlerInterface::invokeAll reindexes arrays » Document that module_invoke_all / ModuleHandlerInterface::invokeAll reindexes arrays
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Moving to 7.x for backport.

  • catch committed 1a33801 on 8.1.x 
    Issue #376391 by mimran, snehi: Document that module_invoke_all /...

  • catch committed 9ad77bf on 
    Issue #376391 by mimran, snehi: Document that module_invoke_all /...
mimran’s picture

mimran CreditAttribution: mimran as a volunteer commented
Status: Patch (to be ported) » Closed (fixed)

This is ported to 8.0.3

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Closed (fixed) » Patch (to be ported)

It was committed to Drupal 8, but still needs to be ported to Drupal 7 :)

pietmarcus’s picture

pietmarcus CreditAttribution: pietmarcus as a volunteer commented
Assigned: Unassigned » pietmarcus
Status: Patch (to be ported) » Needs review
FileSize
376391-29.patch593 bytes

Patched to Drupal 7.

Status: Needs review » Needs work

The last submitted patch, 29: 376391-29.patch, failed testing.

The last submitted patch, 29: 376391-29.patch, failed testing.

pietmarcus’s picture

pietmarcus CreditAttribution: pietmarcus as a volunteer commented
Status: Needs work » Needs review

Tests finally completed successfully. Please review.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks for the backport! Hopefully the Drupal 7 testing system will become more stable...

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Issue tags: +Pending Drupal 7 commit

Marked for commit.

  • stefan.r committed d11d1c2 on 7.x 
    Issue #376391 by mimran, snehi, pietmarcus, jhodgdon: Document that...
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Assigned: pietmarcus » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7, -Novice, -Pending Drupal 7 commit

Committed to 7.x, thanks!

Status: Fixed » Closed (fixed)

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