Add ArrayAccess support to drupal_render()

I'm OK with this, but even if it's only temporary, this is called very very often, and so I'd like to ensure that we don't decrease performance.

And even if it's only temporary, an inline comment explaining the nitty gritty detail about ArrayAccess only being supposed to be supported for drupal_render() but not for Form API would be helpful.

@webchick additionally mentions the lack of phpDoc for the new @param.

Thanks!

Invisible IRC backlog: instanceof is even faster than is_array(), so the performance optimization I attempted in #2 is moot.

What's the extra parameter necessary for?

+      // We want to allow objects implementing ArrayAccess in render API but
+      // not in form API and our forms can contain renderables so return
+      // these only for render API but not form API.

Let's get rid of the 'we want', and explain why form API shouldn't ever see an ArrayAccess object as a valid child. Also there's potentially other uses of element_children() than the render and form APIs, so I would probably remove the 'render API' references, and just make it clear that sometimes you want to allow both but sometimes not.

Better explanation.

just wanted to cross-link another discussion about ArrayAccess that arose while converting to twig:
#1849150-2: The second argument is not an array in format_string() TwigReference from comment.html.twig

Just going through Twig issues, is this issue/patch still relevant?

Updated issue summary.

Closed (won't fix)?

At least this.

I've just bumped into this issue when trying to build some 'creative' renderable arrays.

For what I've seen, doing some experiments, all we need to do for this is just to loosen some restrictions, obviously the ones that require renderable elements to be arrays.

This is an up-to-date 3 lines patch that just removes unneeded restrictions on variables. Please reconsider.

I'm quite in favour of this, though I'd still like that is_object to check for implements ArrayAccess to be on the specific side. Also @Jose Reyero maybe you can address the form api concerns? I don't particularly know what they are (@sun maybe you can clarify why they can't have this as well) but hope that we can just say ArrayAccess for both...

doc blocks should be adjusted accordingly

@joelpittet,
I have no idea what is the problem with form api, if any.

@andypost,
Not sure what needs adjustment, we are just removing that 'array' parameter type but also drupal_render is not guaranteed to work with ArrayAccess -there are other places in the code that won't- and also that cannot be ArrayAccess, since it would need other interfaces like 'Traversable', 'Countable'...

So unless we really want to add some consistency in Drupal core for the use of ArrayAccess as array, and/or convert all array parameters to objects, it may be too late now for that, I don't think there's any point in adding complex documentation for functions.

This is a simple patch for removing unneeded restrictions in the code that block programmers to implement some different solutions.

@Jose Reyero makes sense, +1 to rtbc

Shouldn't this have tests?

@Cottser,

Shouldn't this have tests?

We are not exactly adding support for ArrayAccess here, we're just removing hardcoded restrictions to be able to pass an object instead of an array, so I think the only proof we need so far is that current tests are not failing, so we are not breaking anything.

@Jose Reyero if this change is to stick and not get accidently reverted in the future I'm sure you'd want tests. And the issue title does say it's supporting AccessArray and that is in effect what we are doing.

I'm adding that to this tasks or you'll likely be asking for this feature in a years time again...

@joelpittet,
Good point, I'll add some test.

Added tests, and also removed two more 'array' types in drupal render caching api.

Though these last two changes are not strictly required because the objects can always be crafted to set '#cache' => FALSE, I think they are good just for consistency, so the simplest array object can just be used.

Note in the testing ArrayAccess+Traversable implementation the offset value returned by reference

  /**
   * Implements ArrayAccess::offsetGet().
   */
  public function &offsetGet($offset) {
    return $this->elements[$offset];
  }

This is needed because drupal_render() does stuff like this that need array properties returned by reference:

$elements['#children'] .= drupal_render($elements[$key], TRUE);

Attached 2 interdiffs:
- ..api-diff.txt Includes only the api changes (removed two 'array' parameter types in common.inc
- ..full-diff.txt The full diff including new test.

Updated the issue title, this is not specific of Twig anymore.

I think this change could still happen within Drupal 8. Is there interest?

I'm at WordCamp US right now and saw a presentation reviewing how backwards compatibility has been maintained. It mentioned using arrayAccess to make an object (WP_Theme) out of what had previously been a giant multidimensional array.

We've used ArrayAccess for backwards compatibility before. It's OK but there are limitations like not being able to use any array_* functions, and deep array key setting being tricky - i.e. $foo['bar'] is fine with array access, $foo['bar']['baz']['boo'] iirc there's a workaround but it's not straightforward.

So I think we can try to do this, but it's likely to fail in interesting ways - not sure we'd be able to get to the point in 8.x where we're passing an object to hook_form_alter() for example

It probably makes sense to exhaust our use of RenderableInterface first. There are lots of places like form builders which are self-contained when they're being built, then can just pass an array up to the caller which can then pass an array to alters for now.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

Generally I think we should get as far with this as we absolutely can in 8.x, so that it's a smaller jump to 9.x assuming we go all-objects there.

Move to render system

It probably makes sense to exhaust our use of RenderableInterface first.

+1

catch I've opened #2636468: [META] Implement RenderableInterface on all applicable classes. Can you confirm that such a plan is what you had in mind?