drupal_render() sorting is broken [#364407]

I tried to add content to a node in the following way:

$node->content['aggregator'] = array(
  '#weight' => -1,
  );
$node->content['#sorted'] = FALSE;
// ... build $items array ....
$node->content['aggregator']['info'] = array(
  '#markup' => theme('item_list', $items),
  );

I expected this content to show up above the 'body' element of the node which has the weight 0. But it shows up beneath the 'body' element. drupal_render() does not appear to heed the #weight setting.

I dug into drupal_render and saw that it does execute a sort on $elements but further down the line it does not render from $elements but from $children.

Sort $elements:

  $children = !isset($elements['#children']) ? element_children($elements) : FALSE;
  if (empty($elements['#sorted']) && $children) {
    // $elements are sorted here, but $children are used for rendering
    uasort($elements, 'element_sort');
    $elements['#sorted'] = TRUE;
  }

Render $children:

    // Render each of the children using drupal_render and concatenate them.
    if (!isset($content) || $content === '') {
      foreach ($children as $key) {
        $content .= drupal_render($elements[$key]);
      }
    }

Comment	File	Size	Author
#4	drupal_render_sorting.patch	2.99 KB	catch
#4
#1	drupal_render.patch	1.5 KB	catch
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 26 January 2009 at 16:00

Priority:	Normal	» Critical
Status:	Active	» Needs review
Issue tags:		+Quick fix

File	Size
drupal_render.patch	1.5 KB

Ouch.

This was introduced by #353632: Performance: Do not sort already rendered elements in drupal_render() and is entirely my fault.

Here's a partial revert which removes the (somewhat micro-)optimisation of checking if there's any children to sort before sorting. We should wait until #355236: Refactor drupal_render theming - docs lands before making any more changes like this probably - and after it does that's something we could consider putting back in in a non-broken way.

I'll add tests for this to #354999: unit tests for drupal_render(), hopefully some time tomorrow.

Comment #2

alex_b CreditAttribution: alex_b commented 26 January 2009 at 16:16

This fixes the problem I described in the initial post. I don't move this to RTBC because I don't completely understand the ramifications of sending $elements to uasort() without checking before whether it has children or not. Otherwise fine.

Comment #3

catch

he/him

English

CreditAttribution: catch commented 26 January 2009 at 16:28

A bit more explanation.

The idea of checking $children before sorting, is because in a few cases, $elements gets passed to drupal_render() with no viable children - i.e. every array key is a ['#property'] - so we saved a few calls to element_sort() by checking that there were actually children to sort in the first place. This is minor compared to not sorting things like taxonomy terms every time they're rendered, so we don't lose much simply putting it back the way it was. At least for now, until there's proper unit tests and readable code flow which is being handled elsewhere.