Problem/Motivation

drupal_render_children() is being marked as deprecated so all usages of that function should be removed in Drupal core.

Proposed resolution

Replace drupal_render_children() usage with render().

Remaining tasks

Fix it!
Review!

User interface changes

-

API changes

-

Data model changes

-

CommentFileSizeAuthor
#6 remove-2544176-6.patch2.06 KBjoelpittet
#2 remove-2544176-2.patch775 byteslauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Title: Deprecate drupal_render_children() usage in Drupal\toolbar\Element\Toolbar » Remove drupal_render_children() usage in Drupal\toolbar\Element\Toolbar
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Status: Active » Needs review
FileSize
remove-2544176-2.patch775 bytes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Issue summary: View changes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

I tried seeing if there is another way... nothing came to mind. I don't think trading drupal_render_children() for render() call is a great win because it's an early render either way and both global function calls...

Any other ideas @lauriii?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented

Maybe we need to figure out if we could change the render api to conditionally render children of the render element also as render elements. Otherwise we always have to run an early render in this use case.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
remove-2544176-6.patch2.06 KB

I put this patch on the meta but probably should have put it here. It's the same thing but moving it closer to the source. It also fixes some issues around checking the #wrapper_attributes before building them up which is why the drupal_render_children needs to be called to move the RenderElement's #wrapper_attributes up to the tab/task.s

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/modules/toolbar/toolbar.module
@@ -87,11 +87,20 @@ function template_preprocess_toolbar(&$variables) {
+      if (!empty($element[$key]['tray']['#wrapper_attributes'])) {

@@ -102,7 +111,7 @@ function template_preprocess_toolbar(&$variables) {
+      if (!empty($element[$key]['#wrapper_attributes'])) {

Should we test if tray is array? If its rendered for some reason, this will fatal..

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
Issue tags: +rc target triage

I can't think of a scenario where that would every be a rendered variable. We are early rendering things here but not that doesn't change the $element render array other than adding extra #keys in preRender by reference.

Really don't think we need to do that check. !empty() is just a consistency and performance fix. If it becomes contentious I'll remove it from this patch so we can resolve this.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/toolbar/toolbar.module
@@ -87,11 +87,20 @@ function template_preprocess_toolbar(&$variables) {
+    // ToolbarItem elements.

ToolbarItem fits the previous line but otherwise this is good. Can be fixed on commit.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -rc target triage +rc target

Discussed with the D8 committers and we agreed with making this an rc target.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

Just to follow-up, the reason for this is because it's the last instance of this function (which circumvents the render system and causes caching problems), and this would allow us to mark it deprecated before 8.0.0. It's not "open season" on all cleanup patches. :D

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 8f487d2 on 
    Issue #2544176 by lauriii, joelpittet: Remove drupal_render_children()...

Status: Fixed » Closed (fixed)

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