Part of meta-issue #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1

This small patch allows , for the time being , not to be released like this, adding objects to the render arrays. As http://groups.drupal.org/node/226219 says we plan to replace renderables with objects like #1 contains but while this conversion is ongoing we will the new objects and the render arrays to coexist. Right now adding an object to the render arrays throws an error because of the array checking in element_children().

Files: 
CommentFileSizeAuthor
#26 0150-array-access-26-full-diff.txt2.91 KBJose Reyero
#26 0150-array-access-26-api-diff.txt1.05 KBJose Reyero
#26 0150-array-access-26.patch6.04 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 0150-array-access-26.patch. Unable to apply patch. See the log in the details link for more information. View
#17 0150-array-access-17.patch1.79 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,502 pass(es). View

Comments

chx’s picture

Allows for

-  $form['links'] = array('#theme' => 'item_list', '#items' => $items);
+  $form['links'] = new Drupal\Core\Template\TemplateData('item_list', array(
+    'type' => 'ul',
+    'items' => $items,
+  ));
sun’s picture

Title: Twig-prepatch: Add ArrayAccess support to drupal_render » Twig-prepatch: Add ArrayAccess support to drupal_render()
Issue tags: +Twig
FileSize
1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 36,592 pass(es). View

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.

sun’s picture

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.

chx’s picture

FileSize
2.11 KB
PASSED: [[SimpleTest]]: [MySQL] 36,587 pass(es). View

This is a reroll with comments of #0 because #2 has the condition wrong.

chx’s picture

FileSize
2.11 KB
PASSED: [[SimpleTest]]: [MySQL] 36,587 pass(es). View

With a bonus empty line. Buy one , get two for free!

chx’s picture

FileSize
2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 36,587 pass(es). View

Better argument name and real doxygen.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

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

chx’s picture

Status: Reviewed & tested by the community » Needs review

Erm, I didn't say that. #2 is moot because it is wrong because you do not want to throw an error if an object is there regardless whether you are returning it or not. However, instanceof is not particularly slow because it's a Zend construct not a PHP function call.

chx’s picture

Status: Needs review » Reviewed & tested by the community

X-post.

catch’s picture

What's the extra parameter necessary for?

chx’s picture

form.inc calls element_children w/o the extra argument so that it doesn't see these new objects eventually replacing the render arrays. drupal_render OTOH does see them. That's what the extra argument is for.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+      // 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.

catch’s picture

Issue summary: View changes

Better explanation.

dasjo’s picture

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

Cottser’s picture

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

Cottser’s picture

Issue summary: View changes

Updated issue summary.

MustangGB’s picture

Closed (won't fix)?

catch’s picture

Issue summary: View changes
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: -revisit before beta

At least this.

Jose Reyero’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
1.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,502 pass(es). View

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.

joelpittet’s picture

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

andypost’s picture

doc blocks should be adjusted accordingly

Jose Reyero’s picture

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

andypost’s picture

Assigned: Unassigned » sun

@Jose Reyero makes sense, +1 to rtbc

Cottser’s picture

Shouldn't this have tests?

Jose Reyero’s picture

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

joelpittet’s picture

Assigned: sun » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs tests

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

Jose Reyero’s picture

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

Jose Reyero’s picture

Title: Twig-prepatch: Add ArrayAccess support to drupal_render() » Add ArrayAccess support to drupal_render()
Status: Needs work » Needs review
FileSize
6.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 0150-array-access-26.patch. Unable to apply patch. See the log in the details link for more information. View
1.05 KB
2.91 KB

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.

Status: Needs review » Needs work

The last submitted patch, 26: 0150-array-access-26.patch, failed testing.

stevector’s picture

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.

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

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.

dawehner’s picture

Component: base system » render system

Move to render system

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

+1

stevector’s picture

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?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.