Problem

  • Fatal errors when Config is called in early bootstrap.

Cause

  • Drupal\Core\Config\Config calls into drupal_array_*() functions, which are declared in common.inc.

Proposed solution A

  1. Fork/embed the drupal_array_*() functions into Drupal\Core\Config\Config.

Proposed solution B

  1. 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)

  2. Bikeshed the class name ;)
    • 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.

Proposed solution C

  1. Just move all the drupal_array_*() functions into bootstrap.inc.
CommentFileSizeAuthor
#11 config.array_.11.patch18.07 KBsun
#2 config.array_.2.patch18.08 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

I prefer solution B.

sun’s picture

Status: Active » Needs review
FileSize
18.08 KB

This is what option B would look like.

sun’s picture

Priority: Normal » Major

Unfortunately, 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).

sun’s picture

For full disclosure, I also added the most simple/stupid option C to the summary.

sun’s picture

Issue summary: View changes

Added option C.

sun’s picture

Issue summary: View changes

Updated issue summary.

Crell’s picture

Honestly 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.

gdd’s picture

I'm right on board with crell and agree about gradually deprecating the existing functions as time permits.

sun’s picture

What 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?

aspilicious’s picture

Maybe ArrayHelper I think in the future it may be possible other array helpers will be introduced that aren't linked to nested arrays.

Crell’s picture

I'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.

sun’s picture

In 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).

sun’s picture

FileSize
18.07 KB

cf782bf Moved NestedArray into Component.
4fcfe56 Fixed alphabetic order of use statements.

Crell’s picture

If 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?

sun’s picture

I'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.

sun’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me. Make them novices work for their patches! :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good to me. Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.