drupal_array_*() functions, which are declared in common.inc.drupal_array_*() functions into Drupal\Core\Config\Config.Convert the drupal_array_*() functions into static class methods of Drupal\Core\Utility\NestedArray and make the procedural wrappers call into them.
(could actually live in Drupal\Component)
NestedArray — as direct equivalent of drupal_array_nested_*ArrayHelper — since it contains the array_merge_deep* helper functions, too.DrupalArray — ... (duplicates the Drupal in the namespace though)Note: "array" itself is a reserved keyword, so it always has to be prefixed/suffixed with something.
drupal_array_*() functions into bootstrap.inc.| Comment | File | Size | Author |
|---|---|---|---|
| #11 | config.array_.11.patch | 18.07 KB | sun |
| #2 | config.array_.2.patch | 18.08 KB | sun |
Comments
Comment #1
aspilicious commentedI prefer solution B.
Comment #2
sunThis is what option B would look like.
Comment #3
sunUnfortunately, we need to bump this to major, since a couple of config conversion issues are blocked on this, since code is calling into Config before full bootstrap (i.e., before common.inc is loaded).
Comment #4
sunFor full disclosure, I also added the most simple/stupid option C to the summary.
Comment #4.0
sunAdded option C.
Comment #4.1
sunUpdated issue summary.
Comment #5
Crell commentedHonestly in some cases we could probably get away with A, if the code in question is short enough. Skimming over the patch, it looks like most of them are just complex enough that it's debatable whether it's worth inlining vs. having as a stand alone operation.
Option B feels icky to me, but when we setup Drupal\Component I recall saying that such all-static classes were probably OK there because it moved more code into lazy-loaded classes. We *could* make NestedArray an actual object that's instantiated through the DIC and passed into objects that need those operations but... I can't for the life of me see a benefit to doing so.
So, I'm OK with feeling icky about B. If we go that route we should probably mark the existing functions deprecated and then go through and update the rest of core to call the static methods directly.
C is right out, I think. It doesn't solve the problem, it just reduces its scope a little. Let's for-reals fix this, one way or another.
Comment #6
gddI'm right on board with crell and agree about gradually deprecating the existing functions as time permits.
Comment #7
sunWhat I especially like about this patch and option B is that all of this fancy array handling gets finally bundled into a single place :)
I agree the procedural functions should be deprecated. It should actually just take a Novice follow-up issue to convert all the other calls throughout core (can't get any more novice).
It appears to me we're actually close to RTBC even...? Remaining questions would be:
1) There's really no dependency on any other Drupal stuff in this code, so do we introduce a second Utility bin in Drupal\Component\Utility and move it in there? (or do we just ignore that and keep it in Drupal\Core\Utility?)
2) NestedArray vs. ArrayHelper vs. another name?
Comment #8
aspilicious commentedMaybe ArrayHelper I think in the future it may be possible other array helpers will be introduced that aren't linked to nested arrays.
Comment #9
Crell commentedI'd rather have code in Components if possible, so let's move it there.
These are all utilities for deeply nested array futzing. Will we have a "set" of functions for other array-futzing? If so, and those go in a separate class, then NestedArray makes sense here as a grouping.
Comment #10
sunIn terms of low-level bootstrap/common code in Drupal core includes, these are the only array-futzing functions:
http://api.drupal.org/api/search/8/_array
There are some more about HTTP query parameter/array futzing, but I'd guess that those are going to vanish at some point, as I can only guess that HttpKernel/HttpFoundation contains similar methods already (or doesn't need them).
Aside from these, there are only Options module's options_array_flatten()/_transpose(), but those are totally weird string-to-array-and-vice-versa-futzing functions, which most likely won't be suitable for this NestedArray helper.
On "NestedArray", it is true that deals with deeper nested keys/values, and that's the primary purpose. For the sake of completeness, however, it should be noted that the functions also work with one-dimensional arrays.
ArrayHelper sounds and feels a bit too generic to me; one would wonder why anyone would need any helpers for dealing with arrays. I think the "nested" part clarifies the special purpose (as much as that is possible).
Comment #11
suncf782bf Moved NestedArray into Component.
4fcfe56 Fixed alphabetic order of use statements.
Comment #12
Crell commentedIf the intent is to eliminate the procedural functions, should we also update their docblocks in this patch to just be a stub "Wrapper for NestedArray::whatever(), use that instead", with a @deprecated tag? Or do we save that honor for whoever kills the functions later?
Comment #13
sunI'd leave that honor to the one who kills the functions. ;)
That said, I already considered to update the unit test for the drupal_array_*() functions accordingly, but since all code is still calling into the procedural functions right now, I want to leave the test conversion to the novice issue, too.
Comment #14
sunCreated the Novice follow-up issue: #1705920: Convert all calls to procedural drupal_array_*() functions to Drupal\Component\Utility\NestedArray
Comment #15
Crell commentedMakes sense to me. Make them novices work for their patches! :-)
Comment #16
dries commentedLooks good to me. Committed to 8.x. Thanks.
Comment #17.0
(not verified) commentedUpdated issue summary.