Spin-off from #1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc

Goal

  • Replace all procedural drupal_array_*() function calls with static methods on the Drupal\Component\Utility\NestedArray class.

Challenge

  1. Update all of these calls throughout Drupal core accordingly:
    -  drupal_array_merge_deep_array($arrays) {
+  NestedArray::mergeDeepArray($arrays);

-  drupal_array_get_nested_value(array &$array, array $parents, &$key_exists = NULL) {
+  NestedArray::getValue($array, $parents, $key_exists);

-  drupal_array_set_nested_value(array &$array, array $parents, $value, $force = FALSE) {
+  NestedArray::setValue($array, $parents, $value, $force);

-  drupal_array_unset_nested_value(array &$array, array $parents, &$key_existed = NULL) {
+  NestedArray::unsetValue($array, $parents, $key_existed);

-  function drupal_array_nested_key_exists(array $array, array $parents) {
+  NestedArray::keyExists($array, $parents);
  2. Make sure that each of the files that uses NestedArray contains this use statement:
    use Drupal\Component\Utility\NestedArray;

    This should be at the top of the file. If there is a namespace declaration, it must be below that. If there are other use statements already, add it in the alphabetical order (but do not change other use statements, if they are incorrect).

  3. Remove the procedural functions (entirely).
CommentFileSizeAuthor
#26 drupal8.nestedarray.26.patch41.79 KBunderq
#24 drupal8.nestedarray.24.patch41.8 KBunderq
#20 drupal8.nestedarray.20.patch35.31 KBsun
#15 drupal-1705920-15.patch35.32 KBtim.plunkett
#11 drupal8.nestedarray.11.patch35.5 KBsun
#6 drupal-1705920-6.patch33.99 KBtim.plunkett
#4 1705920.patch32.3 KBAnonymous (not verified)

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Postponed » Active

#1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc is in.

Anonymous’s picture

Anonymous (not verified) commented
Assigned: Unassigned »
Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs review
StatusFileSize
new 1705920.patch32.3 KB

Done.

Status: Needs review » Needs work

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-1705920-6.patch33.99 KB

Patch was partially malformed, and had on windows line ending. After I got it to apply, all of drupal_array was gone.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

@tim.plunkett: I' m tired of Eclipse ( i think it is him, not me) adding \r newlines, or creating wrong patches. Maybe i'm not using it correctly... anyway, could you tell me a tool for checking this previosly to attaching here ? I'm using dreditor, but i cant see those windows line endings on the "Review" tool.

Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Reviewed & tested by the community

Hm, dreditor doesn't pick those up (feature request!), I just noticed it in vim while looking through the patch.

That said, the patch looks good and just passed!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/field/modules/options/options.module
@@ -7,6 +7,7 @@
 use Drupal\field\FieldUpdateForbiddenException;
 use Drupal\entity\EntityFieldQuery;
+use Drupal\Component\Utility\NestedArray;

+++ b/core/modules/file/file.module
@@ -6,6 +6,7 @@
 use Drupal\entity\EntityFieldQuery;
+use Drupal\Component\Utility\NestedArray;

+++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/MiscUnitTest.php
@@ -8,6 +8,7 @@
 use Drupal\simpletest\UnitTestBase;
+use Drupal\Component\Utility\NestedArray;

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/ArrayUnitTest.php
@@ -8,9 +8,10 @@
 use Drupal\simpletest\UnitTestBase;
+use Drupal\Component\Utility\NestedArray;

These aren't added in alphabetical order.

+++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/MiscUnitTest.php
@@ -26,11 +27,11 @@ class MiscUnitTest extends UnitTestBase {
    * Test miscellaneous functions in bootstrap.inc.
    */
   function testMisc() {
-    // Test drupal_array_merge_deep().
+    // Test NestedArray::mergeDeepArray().

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/ArrayUnitTest.php
@@ -8,9 +8,10 @@
+ * Tests the NestedArray helper class.
  */
 class ArrayUnitTest extends UnitTestBase {

It looks like we could and should merge that überwonky MiscUnitTest into ArrayUnitTest? (or at least the array-merge parts of it, if there's more "misc" stuff in that crappy test case...)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Putting use statements in alphabetical order isn't in our coding standards (yet). Maybe open an issue to discuss adding that to http://drupal.org/node/1353118?

That would leave testCheckMemoryLimit() all alone in MiscUnitTest :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.nestedarray.11.patch35.5 KB

I know that the order of use statements is not defined in the coding standards yet, but I wrote the issue summary and specifically asked to add them in alphabetical order - at least for the time being that seems to be the most natural and sensible approach, in order to not significantly increase the havoc.

Performed the final adjustments.

Also renamed ArrayUnitTest into NestedArrayUnitTest.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Re-RTBC then.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Novice

#11: drupal8.nestedarray.11.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

The last submitted patch, drupal8.nestedarray.11.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-1705920-15.patch35.32 KB

Rerolled.

amontero’s picture

amontero
Location Barcelona
commented

How does this relates to #66183: Add helper methods to inject items into a particular position in associative arrays?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks good!

@amontero: Doesn't seem to relate at all to that issue.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: -Novice

#15: drupal-1705920-15.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

The last submitted patch, drupal-1705920-15.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: » Unassigned
Status: Needs work » Needs review
StatusFileSize
new drupal8.nestedarray.20.patch35.31 KB

Re-rolled against HEAD.

amontero’s picture

amontero
Location Barcelona
commented
Issue tags: -Novice

#20: drupal8.nestedarray.20.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, drupal8.nestedarray.20.patch, failed testing.

amontero’s picture

amontero
Location Barcelona
commented
Issue tags: +Needs reroll

Tagging

underq’s picture

underq commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.nestedarray.24.patch41.8 KB

Ok rerolled :)

amontero’s picture

amontero
Location Barcelona
commented
Issue tags: -Needs reroll

Thanks!! The reroll seems OK to me.
Lame nitpick: at line 727 of patch, the comment "Tests the various NestedArray helper class." seems a bit weird to me, sun's original patch deleted "various". But seems not worth to set it to "needs work". At least until no other problems have been ruled out.
Untagging.

underq’s picture

underq commented
StatusFileSize
new drupal8.nestedarray.26.patch41.79 KB

I removed "various" and rerolled patch :)

amontero’s picture

amontero
Location Barcelona
commented
Issue tags: +Feature freeze

Thanks. Tagging.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Feature freeze

Looks good. See also #1356170: Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep()

amontero’s picture

amontero
Location Barcelona
commented

This one was also postponed:
#66183: Add helper methods to inject items into a particular position in associative arrays

dries’s picture

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

Committed to 8.x. Thanks.

catch’s picture

catch
he/him
Primary language English
commented
Title: Convert all calls to procedural drupal_array_*() functions to Drupal\Component\Utility\NestedArray » Change notice: Convert all calls to procedural drupal_array_*() functions to Drupal\Component\Utility\NestedArray
Priority: Normal » Critical
Status: Fixed » Active

This could use a change notification.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Change notice: Convert all calls to procedural drupal_array_*() functions to Drupal\Component\Utility\NestedArray » Convert all calls to procedural drupal_array_*() functions to Drupal\Component\Utility\NestedArray
Priority: Critical » Normal
Status: Active » Fixed

Added http://drupal.org/node/1870678 and updated http://drupal.org/node/1403768

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.