\Drupal\Core\Render\Renderer::doRender() could be lazier about calling Element::children()

Created on 5 June 2013, over 11 years ago
Updated 6 May 2023, over 1 year ago

I noticed this as part of the patch I rolled at #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works .

@Cottser suggested that I could spin it off into its own issue and get it profiled and possibly committed independently.

Basically drupal_render() does this:

// Get the children of the element, sorted by weight.
  $children = element_children($elements, TRUE);

  // Initialize this element's #children, unless a #pre_render callback already
  // preset #children.
  if (!isset($elements['#children'])) {
    $elements['#children'] = '';
  }
  // Call the element's #theme function if it is set. Then any children of the
  // element have to be rendered there. If the internal #render_children
  // property is set, do not call the #theme function to prevent infinite
  // recursion.
  if (isset($elements['#theme']) && !isset($elements['#render_children'])) {
    $elements['#children'] = theme($elements['#theme'], $elements);
  }
  // If #theme was not set and the element has children, render them now.
  // This is the same process as drupal_render_children() but is inlined
  // for speed.
  if ($elements['#children'] === '') {
    foreach ($children as $key) {
      $elements['#children'] .= drupal_render($elements[$key]);
    }
  }

And doesn't use $children anywhere else. Calling element_children() like this is a waste of CPU for #theme renderable arrays.

Feature request
Status

Needs work

Version

10.1

Component
Theme 

Last updated about 12 hours ago

Created by

🇦🇺Australia thedavidmeister

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

  • needs profiling

    It may affect performance, and thus requires in-depth technical reviews and profiling.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • 🇮🇳India nikhil_110

    Attached patch against Drupal 10.1.x

    Patch #27 is not applied for Drupal 10.1.x so Inter-diff file is not added.

    Checking patch core/lib/Drupal/Core/Render/Renderer.php...
    error: while searching for:
    drupal_process_states($elements);
    }

    // Get the children of the element, sorted by weight.
    $children = Element::children($elements, TRUE);

    // Initialize this element's #children, unless a #pre_render callback
    // already preset #children.
    if (!isset($elements['#children'])) {

    error: patch failed: core/lib/Drupal/Core/Render/Renderer.php:404
    error: core/lib/Drupal/Core/Render/Renderer.php: patch does not apply

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    29,302 pass
  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    This feels more like a feature request to me. Or something that should least have test coverage.

    This was profiled 10 years ago but with the new system wonder if it should be profiled again?

Production build 0.71.5 2024