hook_page_alter()

Subscribing.

I need to get my D7 testing install back up to the latest codebase before testing extensively, but this approach is one that I've advocated for quite some time, and support wholeheartedly. We should NOT punt the work of different output formats on the theme system: it's the job of the routing system and this approach gives us loads of extra flexibility while keeping that path clean.

While I'm certinaly in agreement with the broad approach, I'm not sure I like the direction of the implementation here.

@eation - why should the theme layer NOT be the place to transform a given piece of structured data into the chosen output format?

@eation - why should the theme layer NOT be the place to transform a given piece of structured data into the chosen output format?

First, the current patch doesn't prevent information from being piped through the theming system: it just ensures that the theming system isn't our only option for transforming and returning output in a variety of ways.

Our theming system is highly optimized for XHTML (possibly XML) output in which small pieces are overridden to control appearance while the majority of the output uses "default fallbacks." It is currently a bad, bad fit for wildly different output formats being used simultaneously on the same site, especially when one of the formats is something like JSON or serialized PHP.

There's been discussion elsewhere about grafting a new global (or pseudo-global) onto the theming system, a switch that would tell it what format to output in: JSON, Serialized PHP, XML, HTML, PDF, etc. I think this would be a mistake and would put too much responsibility on the templating system. We can use theming in some cases, but it shouldn't be required for everything, any more than everything in Drupal should be a node because nodes are flexible.

Also, to clarify: I'm not opposed refactoring the theme system to expand its capabilities, if that makes sense in the future. I just think that this approach is a sensible, conservative one. Allowing us to build output as structured #renderables and return that is a real, measurable towards greater flexibility, even if every piece isn't yet in place.

Yay! This makes http://drupal.org/project/comment_display obsolete.

Why PNW?

@sun - CNW until #350984: Kick comment rendering out of node module is in.

I think this patch is good - it allows people to work separately on making output formats work and on making menu callbacks return drupal_renderable arrays. Therefore this patch prevents kittens from being killed.

We can move the drupal_render call in to the theme system later. We can move the output format rendering into another place later. We can convert menu callbacks to return arrays later. This just makes it possible to do those in separate patches, therefore not killing kittens.

Actually this should be postponed until that patch gets in.

We can move the drupal_render call in to the theme system later. We can move the output format rendering into another place later. We can convert menu callbacks to return arrays later. This just makes it possible to do those in separate patches, therefore not killing kittens.

Well put, dmitri. Thanks for summarizing it better than I could.

@dmitrig01 - I'm certainly in favor of incremental approaches - but I think we need a clear idea of where we want to end up.

@eaton - I would assert that any thorough approach to rendering a node fully as JSON or atom vs. xhtml is going to need a parallel function to each current theme function. The approach suggested by chx, which is better than what I had originally proposed, would be to populate (once per page load) a mapping of partial theme hooks to full theme hook. e.g. theme() would simply look in this mapping for 'page', and based on that tries to invoke (e.g.) theme_page() vs. theme_json_page(). Obviously we are special casing xhtml to be an empty prefix to avoid the work of re-writing all existing implementations.

The nice part about this is that it wold allow for very granular control - e.g. the blocks could be rendered xhtml inside a JSON wrapper (theme_json_page()) for use in remotely refreshing them block contents.

@pwolanin Completely agreed on composited theming. I had a long discussion on Skype with Moshe about the need for it for other reasons.

And it's on again.

subscribing

Rerolled, and it works for me.

Better code style. Have no idea if tests actually pass.

Ok, preview works and all node tests pass.

w00t at the drupal_alter('page')! However, I'd like to propose moving the if statement (or at least the elseif and else components) into common.inc, drupal_build_page(). That way, drupal_build_page can also do drupal_alter('page_$url') without busying index.php.

This looks kind of funny:

-
+ddebug_backtrace();

Ok, removed. Also put the page rendering into a separate function, drupal_render_page, so we can add the extra drupal_render later.

Also I've coded some of the conversion for forms - #353069: Make drupal_get_form to return unrendered forms

Thanks for fixing the inconsitency. this patch fixes admin pages

a minor point - I really dislike the use of $render as a variable name - it's suggests an action, not data.

Ok, renamed $render to $page and removed it in a few places.

it's ready for review

After talking to sun and chx, changed around a bit more stuff for #type => list. all the same besides that (blog tests pass). Also batch isn't ready for conversion yet

Fixed failure

Had a quick look at moshe's patch in #30 just to see where it was at. dmitrig01 posted an update but I don't have time to re-review so these may not be relevant.

theme_list()'s PHPDocs seem a little awkward. I'd rather have the required/optional bits up front:

+ *     - #items. Required array of items as expected by theme('item_list').
+ *     - #title. Optional title string which prints above the list.
+ *     - #list_type. The type of list to return (e.g. "ul", "ol"). Defaults to "ul".
+ *     - #attributes. Optional array of attributes as expected by theme('item_list').

The changes to the taxonomy module remove the n.sticky, n.title, and n.created fields from the queries. Don't those need to be there for the ORDER by to work on pgsql?

Great patch! This will enable all kinds of functionality regarding different output formats. Great point is also that contrib can now simply conditionally redirect page rendering to a custom theme function, doing there whatever is suitable. Awesome.

+ // Move some variables to the top level for themer convenience and template cleanliness.
+ $names = array('show_blocks' => TRUE, 'show_messages' => TRUE);
Shouldn't we move the defaults to drupal_render_page or system_elements so that this can be altered in hook_alter_page?

Good idea, done

While I realize this could be done later, I think we should correct the basic flaws in drupal_render_page() now.

The most important problem is that we anticipate handling a variety of structured data that might already have a top-level #theme defined. Hence, the setting of '#type' => 'page' is not generally going to work for the top-level array. Rather the page needs to be an element containing the passed in $content array.

I would also suggest that it would be useful to set the #title and possibly some other properties before the drupal_alter() call.

This is what I had in http://drupal.org/node/134478:

+/**
+ * Build a structured array with page content and theme parameters.
+ */
+function drupal_build_page($content, $show_blocks = TRUE, $show_messages = TRUE, $success = TRUE) {
+  if (!is_array($content)) {
+    $content = array('#markup' => $content);
+  }
+  $elements = array(
+    '#title' => drupal_get_title(),
+    '#theme' => 'page',
+    '#show_blocks' => $show_blocks,
+    '#show_messages' => $show_messages,
+    '#success' => $success,
+    'content' => $content,
+  );
+
+  global $theme;
+
+  $regions = system_region_list($theme);
+  // Load all region content assigned via blocks.
+  foreach (array_keys($regions) as $region) {
+    if (!isset($elements[$region])) {
+      $elements[$region] =  array();
+    }
+    // Prevent left and right regions from rendering blocks when 'show_blocks' == FALSE.
+    if (!(!$show_blocks && ($region == 'left' || $region == 'right'))) {
+      $elements[$region]['blocks'] = array('#markup' => theme('blocks', $region));
+    }
+  }
+
+  // Alter any part of the page before it's rendered.
+  drupal_alter('page', $elements);
+  return page_elements_builder($elements);
+}
+

As you can see above, doing this allows you to pass in params (like $show_blocks) - this codein the patch is rather ugly I think:

+  // Move some variables to the top level for themer convenience and template cleanliness.
+  $names = array('show_blocks', 'show_messages');
+  foreach ($names as $name) {
+    $variables[$name] = $variables['content']['#'. $name];
+  }
+  $variables['content'] = &$variables['content']['#children'];
+ 

tested the patch on postgresql and the change to the taxonomy queries totally break it:
PDOException: SELECT DISTINCT(n.nid) FROM {node} n INNER JOIN {term_node} tn0 ON n.vid = tn0.vid WHERE n.status = 1 AND tn0.tid IN (?) ORDER BY n.sticky DESC, n.created DESC LIMIT 10 OFFSET 0 - Array ( [0] => 11 ) SQLSTATE[42P10]: Invalid column reference: 7 ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list in taxonomy_select_nodes() (line 1259 of /Users/amorton/Sites/dh/modules/taxonomy/taxonomy.module).

Those benchmarks make a long issue even longer and unreadable. Please attach them instead.

I agree with pwolanin in #37. We shouldn't make the main content the root element of the page array. This is limiting in several ways, as we then cannot easily have more elements at the same level as the page's main content, for example blocks and regions. See the attached patch

The attaches patch does this:

• The main content is now always in $page['content']
• To still be able to pass properties like #show_blocks directly to the root page array, one can stick them into #page_properties of the content array
• Blocks are loaded into the page array in drupal_render_page, keyed by region. I am working on a follow up patch that puts blocks into renderable arrays at that point, making them alterable by hook_page_alter. This wasn't possible with the previous patches. Also, in a follow up patch, drupal_set_content could then be replaced by hook_page_alter. Heck, we could even make block.module implement hook_page_alter and make it put the blocks into the regions there, thus making block.module possibly optional
• I added an optional property #concatenate_children. If an element has this set to FALSE (TRUE is default), then drupal_render will not concatenate the children into a single string, but instead makes #children an array containing all the rendered children. This gives full control over all elements to the theming function. This is used for the page element, so in template_preprocess_page we then import all children into the $variables array, making the regions and main 'content' available as theming variables.

I'm leaving at CNW because I haven't dealt yet with #38. Also, there are some notices on 404 pages, not yet sure why as I mostly just copy-pasted the code for block rendering from template_preprocess_page. Maybe the theme system isn't yet initialized at that point of time?

Fixed the warnings in my last patch.
Setting to CNR, even though it needs work on postgres but this is not among the most important parts of the patch and the rest definitely needs review.

And now blocks are put into renderable arrays as well, making blocks and regions alterable in hook_page_alter. So blocks are now rendered during the drupal_render call in drupal_render_page as well. All block tests pass.

New patch!
Most important changes, after discussion with moshe and dmitrig:

• We now have a function drupal_decorate_page(). One can simply call it with an array as argument, and during page rendering, this will then be merged into the main page array. This can for example be used to disable blocks (by calling drupal_decorate_page(array('#show_blocks') => FALSE)), or to inject new content into regions, thus replacing drupal_set_content. (drupal_set_content() is still kept in the patch because it is used in theme maintaince mode, which we haven't touched at all yet).
• No more #page_arguments, use drupal_decorate_page() instead
• Block rendering is now done in block.module via hook_page_alter!

I don't particularly like the model of drupal_decorate_page(). We don't usually handle decorators in Drupal using functions we call to make the change. Rather, we usually have hook_*_alter() hooks that get the data passed by reference. Why not hook_page_alter()?

Well, hook_page_alter gets called for each and every page view, while drupal_decorate_page() can be called by a menu callback function or whatever. So, if I want to disable blocks in my menu callback or want to add some content to some region, it's very simple with drupal_decorate_page(), while in hook_page_alter, I'd either have to check arg() or set some global in my menu callback, which is both kinda ugly IMO. drupal_decorate_page() really gives us lots of flexibility without the drawback of having to implement a hook_page_alter() which gets called on every page view.

So, drupal_decorate_page() has a quite some advantages as its much cheaper than hook_page_alter. And I cannot see any disadvantages about it.
We also have hook_page_alter(), of course, which used by block module in the latest patch to add the rendered blocks to the page array. drupal_decorate_page() instead is used by 404s to disable blocks and by block module to add region headers on admin/build/block, thus replacing drupal_set_content().

Constructing the pages in the menu callback with a bare page array is nice.

I'd still keep drupal_decorate_page(), though, for example to avoid something like that:
+ $item = menu_get_item();
+ if ($item['path'] == 'admin/build/block') {
+ $description = '

';
+ $page[$region]['blocks']['block_description'] = array('#markup' => $description);
+ }
What is the reasoning against drupal_decorate_page(), other than that so far we haven't done something like this in Drupal this way?
drupal_decorate_page() is a very neat little function IMO.. saves LOCs easily and can be called literally by anyone from anywhere. So it makes menu callbacks less special, which is a good thing IMO. And the function is just 4 lines.

You moved filling out empty regions into block.module. Why? I'd keep that in common.inc. Regions really are part of the base system, while block.module should just put its blocks into the regions. If we move into the direction of maybe making block.module optional in a follow up page, region handling should stay in common.inc IMO.

Otherwise the patch looks good.

Since i'm not really qualified to review this because i worked on the patch, i'm not going to review it, but here's my one-word response: "wow"

Heh, nice humble tag. ;)

This looks really cool. I only had 20 minutes to look at this so far, but here's what stood out to me (mostly a "Coder module" pass at this point):

+ * @param $page
+ *   A string or array representing the content of the page.

Seems odd that this accepts a string or an array, but dmitri told me that this is so that we can have backwards compatibility until all pages are converted in later patches, so works for me.

+        // If #concatenate_children is set to FALSE, the rendered children of the
+        // element are not concatenated into a single string, but instead kept as
+        // a keyed array of rendered string in the #children property of the parent
+        // element. The parent's element theming function is then responsible for
+        // printing out all elements in the #children array.

Um. What? :) Could we English-ify this a little?
a) What is the normal behaviour that is done if this isn't concatenated?
b) Why would I want to set this value?
c) The volume of comments here makes me feel like it's really important, so we should add a sentence at the top summarizing why it's important to have 5 lines describing it.

+ *     - #items. An array of items as expected by theme('item_list'). Mandatory.
+ *     - #title. A title which prints above the list. Optional.
+ *     - #list_type. The type of list to return. Defaults to "ul".
+ *     - #attributes. An array of attributes as expected by theme('item_list'). Optional.

Please separate the values from their descriptions by colons, rather than periods for consistency with other places we list options like this.

+  return theme('item_list', $elements['#items'], $elements['#title'], $elements['#list_type'], $elements['#attributes']);
+}
+/**

Add a newline between } and /* please.

+    if(empty($variables[$region_key])) {

Need a space between if and (

+  $variables['block'] = $variables['block']['#block'];

Just curious, why is this $variables['block'] and not $variables['elements']['block'] like node and teaser above?

+    foreach ($list as $key => $block) {
+      $output[$key] = array(
+        '#theme' => 'block', 
+        '#block' => $block, 
+        '#weight' => $weight
+      );

Please add a comma at the end of $weight.

+    $page['content']['pager'] = array(
+      '#markup' => theme('pager', NULL, variable_get('default_nodes_main', 10)),
+      '#weight' => count($nodes),
+    );

It's rare, but pages do sometimes have multiple pagers on them. Should we make this an array with $element as the key?

+  $build = $node->content;
+  $build += array(
+    '#theme' => 'node',
+    'node' => $node,
+    'teaser' => $teaser,
+  );

I'm having some trouble here trying to figure out why sometimes things have # symbols before them and sometimes they don't. Could you explain this?

+    $build["nid_$node->nid"] = node_view($node, $teaser);
+    $build["nid_$node->nid"]['#weight'] = $weight;

Would maybe $build['nodes'][$nid] be better, so node IDs could be parsed out more easily if required?

+      '#markup' => t("Opinions expressed on this page belong to it's authors, and not to Acme, Inc."),

it's -> its

We'll also need some documentation .. somewhere... that describes what all the system properties of the $page object are. Maybe in the PHPDoc of drupal_get_page(). That might help me feel a bit less lost than I do now.

Also, now that tests are passing again, could we get a fresh set of benchmarks to see how we're fairing?

Looks like webchick beat me to this but we didn't overlap too much so I'll stop here and let these points get addressed.

In drupal_not_found() the "+ // Custom 404 handler." comment seems like it should be integrated into a paragraph with the next comments. Same goes for "+ // Custom 403 handler."

There's also trailing whitespace in drupal_not_found() that would be nice to remove.

You didn't change them but since we're in here we should document that drupal_access_denied() and drupal_not_found() could return strings or arrays depending on the error menu handler).

drupal_get_page()'s PHPDoc block starts with multiple sentences. It should start with a single sentence describing the function and then additional paragraphs. Please explain what decorating is, I don't think we can assume people will know. Some @see links to other functions like drupal_render_page() would be helpful.

Rather than:

+    $body = array('#markup' => $return);
+    $page['content'] = $body;

Can't we just do $page['content'] = array('#markup' => $return); ?

drupal_render_page()'s PHPDocs should describe the format of the page array. I think we're going to need some extensive documentation on this function. Need PHPDocs explaining what it returns.

Webchick had balked at:

+        // If #concatenate_children is set to FALSE, the rendered children of the
+        // element are not concatenated into a single string, but instead kept as
+        // a keyed array of rendered string in the #children property of the parent
+        // element. The parent's element theming function is then responsible for
+        // printing out all elements in the #children array.

and I'll agree it's pretty hard to parse. I'd suggest describing what the default behavior is first then describe why you might not want that and then how setting it to false will help. e.g.:

By default drupal_render() processes all the element's children and converts them into a string. In some cases it is desirable to selectively render parts of the array and leave the final string conversion to a theme function. If the #concatenate_children property is set to FALSE the elements children are rendered and stored in the #children property as a keyed array of strings. The element's theming function is then responsible for printing out all elements in the #children array.

moshe, i disagree on putting all the docs in to the page_example.module. that's the kiss of death for docs since they're not updated with the core patch. it's critical that the docs get attention from the people who developed the code rather than becoming an after thought. that was the whole reason for moving the hook documentation into core. to ensure that the docs get the same scrutiny as the code.

Up in comment #33 I suggested that:

+ *     - #items: An array of items as expected by theme('item_list'). Mandatory.
+ *     - #title: A title which prints above the list. Optional.
+ *     - #list_type: The type of list to return. Defaults to "ul".
+ *     - #attributes: An array of attributes as expected by theme('item_list'). Optional.

could be improved.

I hate to be a broken record but this will fail on pgsql because the ORDER BY terms aren't in the select list:

+  $nids = pager_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.type = 'blog' AND n.uid = %d AND n.status = 1 ORDER BY n.sticky DESC, n.created DESC"), variable_get('default_nodes_main', 10), 0, NULL, $account->uid)->fetchCol();

Same goes for:

+  $nids = pager_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.type = 'blog' AND n.status = 1 ORDER BY n.sticky DESC, n.created DESC"), variable_get('default_nodes_main', 10))->fetchCol();

I think:

-  // We only care about the node if it's been marked private. If not, it is
+  // We only care about the node if its been marked private. If not, it is

is incorrect because we're using the contraction it has rather than the possessive. It would be best to just expand it to "it has".

Need to add PHPDocs for @params and @return to node_show().

The @param tags for node_build_multiple() use data types before the variable names. That's not the current style through core, the type is typically specified in the description. Personally I like the types but it's inconsistent so we should follow the rest of core. And I'm not sure if api.module would parse them correctly.

up on #53 I'd asked about:

+  if (is_string($return)) {
+    $body = array('#markup' => $return);
+    $page['content'] = $body;
+  }

can we shorten that up?

if #items is really a mandatory parameter for theme_list() then we shouldn't set a default. that looks like hand holding. let it fail with some noise right away.

can we turn this into one paragraph?:

+  // Import children of the page array as variables.
+  // This contains the main 'content' and the regions (with rendered blocks).

lets make this a proper sentence by adding a period.

   // Add favicon

this should either be a paragraph or separate comment blocks:

+  // Populate all block regions.
+  // The theme system might not yet be initialized. We need $theme.

the code and comment for this seem like they could use some tweaking to make them read teh same way. perhaps transforming the tests?

+    // Prevent left and right regions from rendering blocks when 'show_blocks' == FALSE.
+    if ($page['#show_blocks'] || ($region != 'left' && $region != 'right')) {

hook_page_alter()'s PHPDocs could use a @see to drupal_render_page()... actually drupal_render_page() could also use one to hook_page_alter().

In taxonomy_term_page() we're adding the

I think #concatenate_children should be omitted from the patch. If nothing else, if this is the minimal, first, patch, it should not be needed anywhere.

I defined a separate version of drupal_render in the page-randering patch to do the same thing (return an array containing all the child elements). However - the final output of this process will always be a concatenated string, so chx persuaded me that this was not the right approach.

talking with Moshe I see I misunderstood a little what that flag is doing - it only affects a single level of the tree, it's not passed down recursively.

So, one could achieve the same effect by having regions be # properties of the page, but it's a toss-up which is less ugly.

One minor thought - if the meaning of the flag was reversed perhaps the logic could be simplified, e.g. something like:

+        if (empty($elements['#preserve_children'])) {
+          $content .= drupal_render($elements[$key]);
+        }
+        else {
+          $content[$key] = drupal_render($elements[$key]);
+        }

Why is #concatenate_children not the right approach? I can see it useful in other situations as well, to be precise whenever you know what children you expect (as we do here - it's the regions) and want to handle putting them into a template wherever you want by yourself. What's the reasoning against it?

I suggest to make $content an optional argument to drupal_get_page(), and assigning it to $page['content'] there if not empty. In 90% or the situations we will want to return the page array with just the content array and no additional parameters from the menu callback. In all these situations we could then simply write
return drupal_get_page($content);
instead of

 $page = drupal_get_page();
$page['content'] = $content;
return $page; 

Saves some LOCs in many situations as soon as we start to convert more menu callbacks to return arrays.

I still somehow like drupal_decorate_page as in my patch in #43, but it doesn't matter a lot (we have the functionality there still)

Otherwise, the patch really looks great.

Cross-posted with pwolanin, #concatenate_children seems to be adressed, great! Reversing the logic is fine with me.

I like Frando's suggestion in #65.

My impression has always been that drupal_render() should spit out a fully rendered string. Inside the context of drupal_render(), template files control how the individual elements are output, and how they're wrapped in decorative HTML. Adding a new override flag to change drupal_render's behavior seems a little strange.

If you want to exercise greater control over what happens inside of it, you can call drupal_render on individual children, or implement theme functions and preprocessors that pre-render the chunks explicitly, then hand them off to a template. Am I missing something?

Other than that detail, wow, Freakin' sweet.

I just ran benchmarks with #69, and performance seems to be unaffected by the patch, which is great.

HEAD on /node with 90 nodes (no caching):
11.72
11.63
11.68

HEAD + Patch #69 on /node with 90 nodes (no caching):
11.60
11.67
11.62

(Numbers are requests per second (mean) as reported by ab -n 200 -c 10)

Patch #69 was broken for menu callbacks returning strings. Fixed that in the attached patch.
I also moved strings returned by menu callbacks to $page['content']['main']. This makes it possible to add stuff in the content region above the main content (by setting a negative weight on the element added to $page['content']). This wouldn't be possible otherwise for menu callbacks returning strings (theme_markup adds all children to the end of the markup string). Also fixed the hook_page_alter example in system.api.inc so that it would actually print something.

I just had a lengthy chat with eaton about the hell that drupal_render() internals are, and while we both agree that a general cleanup of drupal_render() would be great, he also agrees that the #concatenate_children flag in this patch is the way to go for page rendering by now. We thought about renaming it to #preserve_seperate_keys and making it FALSE by default, which is OK with me as well (as is the current name).

I wanted to suggest a better property name than #concatenate_children and #preserve_separate_keys, since both are all Greek to me. However, I did not even completely understand the purpose of the property, so I asked Frando to explain it to me in IRC:

Normally, for an element that has #type set and #theme empty, drupal_render iterates over all children, drupal_renders them and concatenates the results into a single string. This single string is then put into $elements['#children'], and theme_{$elements['#type']} is expected to print this out.

With concatenate_children set to FALSE, $elements['#children'] is an array instead, keyed by child name, with the value being a string (the rendered child element). Then, the theme function of the type is in charge to actually print all the elements in there. This is what's done for the page element - all regions are rendered and the strings are in $page['#children'] then, and in template_preprocess_page all items in $page['#children'] will then be imported into the top level of $variables.

After reading this explanation a second time, I wonder whether it is a good idea to store two different data types in the same property (#children) depending on another flag. After all, our #properties can be compared with function arguments, and we usually try to avoid different data types for the same function argument.

I have been thinking about this more - and I really think #concatenate_children should be omitted. It introduces a variable return to drupal_render() which is just bad design. Also, and more importantly, we eventually want to be rendering the content array in the template to give themers the option to render different parts in a different order. Thus, we should move the call to render at least to the preprocess function.

I have been thinking about this more - and I really think #concatenate_children should be omitted. It introduces a variable return to drupal_render() which is just bad design. Also, and more importantly, we eventually want to be rendering the content array in the template to give themers the option to render different parts in a different order. Thus, we should move the call to render at least to the preprocess function.

I agree, and that's exactly what I said we needed for this patch to be a killer. Frando, moshe and I spent about an hour or two hashing through the details, though, and we revealed a fundamental gotcha in drupal_render()'s internal workflow: the theme function for a given #element type receives its children pre-rendered, but the #theme function for an individual discrete element can't control the 'wrapper' markup that goes around the element. #355236 has been started to resolve this problem, and I'm whipping up some omnigraffle stuff in a bit.

A short-term compromise would be to use a drupal_render_children call or something like that rather than calling drupal_render() at the top level directly, until the problems #355236 deals with are resolved.

I'm working on re-rolling the patch per my comments and discussing w/ Moshe.

Now that I'm back from vacation, here's why drupal_decorate_page() and all functions like it are bad:

* Functions that store things in static variables and regurgitate their static variables when you call them with magic arguments are just bad imitations of static classes. (I'm looking at you, drupal_add_js(), drupal_add_css(), drupal_set_message().)
* When you call functions with side-effects, it makes your code harder to cache and test. To cache or test code that calls side-effect functions, you have to unreliably probe for the side-effects. For example, if function P adds CSS file M.css, you know you should have M.css in the list of CSS files to be added to the final page. But if function Q also adds M.css to the page and is sometimes called before function P, you can't easily verify that function P is actually adding M.css. It's possible that your test will falsely pass or fail depending on whether function Q ran before P. This is a problem for good unit tests, which should be randomizable in order without affecting results. Finally, attempts to cache the portions of a page -- including (for example) the CSS files they require -- are thwarted by the inability to accurately determine the what CSS files a block (for example) decided to include. At best, you end up running a crude "diff" on the before and after states of the functions storing the side-effects. The "diff" approach doesn't necessarily even work if your side-effect functions are idempotent, like drupal_set_title().
* When you call functions with side-effects, it makes your code dirtier and slower to test because you have to ensure a starting clean environment and clean up any mess created by the side-effects. It's much easier to call a function that puts what it wants in its return value and verify that, for example, function P includes M.css in the $page array it returns.
* Side-effect functions are hard to control. If you run drupal_set_message() from a function returning a Form API array, the message may get set during form processing in addition to when the user first sees the form. By encapsulating things in return values like a $page array, we can better control the propagation of things like messages and titles. For forms, we *do not* want messages to display when a form is being loaded for validation purposes only. Similarly, when we use functions like drupal_set_title(), we currently have little control over their effect and have to resort to awkward mechanisms like calling node_page_view() if (and only if) you want to *also* set the page title.

For anything we'd like to do with drupal_decorate_page(), I'm sure we can find a superior architecture that avoids the problems above.

David - two comments
1. (minor) drupal_set_message actually uses $_SESSION
2. are you arguing for a global $page? that way, modules like drupal_add_js and such can alter it.

@dmitrig01: I'm not arguing for any approach other than avoiding a litany of side-effect functions.

ok, then i'll begin arguing for global $page; :P

I'm a fan of a built-then-altered $page variable, as opposed to a global "add this to the current page". However, that means that every module will need access to some sort of get_current_page() function. Isn't that global state? I suppose it's still preferable to the tricky 'call it with no params and get the data back' model we have.

Just spent some time taking this patch for a spin; I'm very impressed with the changes. There are a lot of subtle tweaks that were previously very difficult that can now be made simpler: modules that pull a small unrendered chunk of the normal $content region and put it into another area of the page (like a block or a header) when it's present, for example.

I stuck a simple hook_page_alter() implementation in a module that prints dsm($page), and surfed around a sample install: the modified structures are good. They give us a sound basis for future refinement (making more pages return structures rather than pre-rendered text, putting more page metadata in the $page variable for potential altering, and so on).

What rough edges there are in the details of the rendering for this patch need to be solved in drupal_render itself, IMO. This patch as it stands is a great improvement, the impact on developers writing module code is minimal, and it opens up new doors for future work. Big thumbs up.

I'm running the full suite of tests right now; pending the green light from simpletest, I give it a big thumbs up.

Aside from trivial coding changes, my very through review contained one more important change: we now use #prefix and #suffix as much as we can. Earlier benchmarks showed no speed up or decrease. And we have #353632: Performance: Do not sort already rendered elements in drupal_render() upcoming. Given my earlier review, opinion of the testingbot and Eaton's and the benchmark results, I am honored to greenlight this rather fine patch.

Patch review (lightly edited IRC log) attached for posterity.

it's minor but hopefully one of the committers can remove all the trailing whitespace that the patch introduces... yuck. i'm sure that would knock a couple of KB off the patch.

Applied the patch then ran all the tests on pgsql and they look good. There was one failure (Database > SQL handling tests > First record inserted as expected) but I don't think it's related to this patch.

Note that this is a functionality review of the patch, not a code review. I'll need to do one more of those before I'm comfortable confirming RTBC status.

The biggest thing I've been missing with this patch is an overall sense of what a $page object actually looks like. So I used Jeff Eaton's trick and created a module that implements a hook_page_alter() and sticks a textarea at the top of all pages with the contents of the $page array in it. This allowed me to click around and see how it changed. The module is attached to this issue for anyone else who is more "visually" oriented and would like to test.

Here's what it ends up looking like on most pages on a default install:

array (
  '#description' => NULL,
  '#attributes' => array (),
  '#required' => false,
  '#show_messages' => true,
  '#show_blocks' => true,
  '#type' => 'page',
  'content' => array (
    'main' => array (
      '#markup' => '(The rendered HTML content area of the page.)',
    ),
  ),
  'left' => array (
    'blocks' =>  array (
      'user_navigation' =>  array (
        '#theme' => 'block',
        '#block' => stdClass::__set_state(array( // I don't know what that means. :)
           'bid' => '2',
           'module' => 'user',
           'delta' => 'navigation',
           'theme' => 'garland',
           'status' => '1',
           'weight' => '0',
           'region' => 'left',
           'custom' => '0',
           'visibility' => '0',
           'pages' => '',
           'title' => '',
           'cache' => '-1',
           'enabled' => true,
           'page_match' => true,
           'subject' => 'a',
           'content' => '(The fully rendered content of the block.)',
        )),
        '#weight' => 1,
      ),
      '#sorted' => true,
    ),
  ),
  'footer' => array (
    'blocks' => array (
      'system_powered-by' => 
      array (
        '#theme' => 'block',
        '#block' => 
        stdClass::__set_state(array(
           'bid' => '3',
           'module' => 'system',
           'delta' => 'powered-by',
           'theme' => 'garland',
           'status' => '1',
           'weight' => '10',
           'region' => 'footer',
           'custom' => '0',
           'visibility' => '0',
           'pages' => '',
           'title' => '',
           'cache' => '-1',
           'enabled' => true,
           'page_match' => true,
           'subject' => '',
           'content' => '(The fully rendered HTML content of the block.)',
        )),
        '#weight' => 1,
      ),
      '#sorted' => true,
    ),
  ),
)

Things that doing this uncovered:

1. Node-related pages (empty front page, node page, front page with nodes in it) are missing the ['main'] index on $page['content']. It's just $page['content']['#markup']. I'm pretty sure this is a bug, since everywhere else it is not like this. At the very least, it's a DX problem because you need to deal with node pages and "other" pages differently, so I'd like to see this made consistent. Keep $page['content']['nodes'] though; that's really useful.

2. The array has a lot of very useful things in it, but also some notably missing things: hook_help() text, messages set by drupal_set_message(), breadcrumbs, and page titles. When I think "page" I think the entire contents of the browser window, so I would assume that I could alter these elements as well (for example, $page['content']['help'], $page['content']['messages'], $page['#title'], and $page['#breadcrumbs'] or $page['breadcrumbs']). It's confusing to me that I can alter things outside of the $content area (block regions, and blocks within those regions) but not things printed out before $content.

3. The side effect of placing this kind of power at developers' fingertips is that people are going to end up using it for absolutely everything: altering the contents of blocks (and creating new ones on-the-fly), adding or modifying links on nodes, completely swapping out a form on a given page, etc. In other words, stuff we have the hook system for. I see that the PHPDoc on hook_page_alter() alludes to when it's appropriate to use hooks and when it's appropriate to use this, but we should flesh this out a bit more so it's clear to new developers "If you want to do task X, this is the hook for you. If you want to do task Y, use this other hook." I think we should also mention that the entire page's HTML is rendered at the point this hook fires, so you gain more by altering things earlier in the cycle. It would also be nice to have a sample output of var_export($page) in the PHPDoc there so that I didn't have to write a module to see what this hook does. This should explain the philosophy behind the organization of indexes and what standards people should follow when adding in their own. In summary, more betterer comments on hook_page_alter(), please. ;)

4. I'm also wondering if we shouldn't store flags like #show_blocks and #show_messages within their own sub-index (like $page['#settings']). One thing that's nice about this array the way it is now that it's pretty clear which are "internal" properties (#attributes, #required, etc.) and which ones are meant to be monkeyed around with $page['content'], $page['left'], etc. I forsee us adding more of these little boolean properties in the future to turn on/off behaviours of a page, and it might be nice to collect them under one area. Makes the documentation easier to write ("see $page['#settings'] for a list of flags," vs. "#foo does this and #bar does that and...").

5. Minor, but what's the purpose of the #required flag on $page? Is this just automatically added to the array as a side-effect of drupal_render()?

Also, I renamed this issue both because I think it might help get some more people looking at it, and also because I could never remember what keywords to use to find it again. :P

Oh. And let's get rid of the trailing whitespace while we're at it, as noted in #89.

Ok. Now that I have had a cup of coffee, I realize that regarding #1, the pages that have $page['content']['main'] are in actuality legacy pages, which is why they are all consistent. So I should actually have been reviewing pages like node/* and taxonomy/term/*. Sheesh. :P Sorry. :P

I've attached a few var_export()s of various pages for comparison. I've also updated page_alter.module so it works in any page context, which is accomplished by doing Unholy Things (tm) -- creating a block on the fly and inserting it into the header region. ;)

no. 1:
well, on node pages, we return an array containing the individual nodes. So in hook_page_alter, I think you could just add stuff to $page['content'] with a low weight and it will be rendered above the nodes. Later on it then gets all rendered into #markup in $page['content']. We could modify drupal_get_page, though, to put the content returned by menu callbacks into $page['content']['main']. I don't think it's really required, but it would improve consistency between menu callbacks returning strings and those returning arrays a bit.

no. 2:
I think we should leave that up to an follow up issue. There's lot of stuff that could be moved from preprocess_page to drupal_render_page or page_alter, but there are certainly some items where this will lead to discussion, so I'd vote to first get in the basic stuff (which this patch is) and then move the rest around later.

no. 3:
Well, as a general rule of thumb I'd say "If it's possible to do what you want to achieve earlier in the page generation process, by using another hook, do it there. If it's not possible, use hook_page_alter".

no. 4:
I just fear that we're kinda getting into arbitrary decisions here. What should be moved into #settings? What should stay top-level? That would give us three realms to put "stuff" into (#property, children and #settings['property']). If we go this route, we really gotta make up some rules for it. Personally, I'd be more interested in that patch that was putting a leading underscore to internal FAPI attributes, but I have no idea if that one is still alive.
If we happen to add much more properties to the $page array (IMO, that would depend on the route we go for different output formats etc), I would be in favor of the #settings array, to keep things more seperated from "internal" FAPI properties.

no. 5:
I think this is just the default value. All elements with #type set and defaults merged in have it.

After analyzing the various array structures, the biggest problem I have with this patch right now is the lack of consistency, which I think is going to cause confusion for new developers.

An example of the kind of consistency I think we should be shooting for is the way blocks are represented in the $page. All block regions have named indexes in the $page array, and the blocks within them are contained in a 'blocks' sub-element, sorted by weight. So if you want to target a given block on the page, you're always going after $page[{region name}]['blocks'][{module name}-{delta}]['#block']. This is consistent, and easily understandable. I can stand up in front of a room of new Drupal developers and tell them this and they will get it.

However, with $page['content'], it feels like your only recourse is to use something like page_alter.module to know what on earth is available to you:
- The node/taxonomy listing pages have a $page['content']['nodes'] property, but a single node page does not.
- If you're on a single node page, then properties that used to be at $page['content']['nodes'][{nid}]['body'] are now at $page['content']['body']. But there is nothing to distinguish this being a *node* body as opposed to just a generic "body" of a page.
- Or, is this lack of distinguishing by design? If it is, why don't the content listing pages and legacy pages also have a $page['content']['body'] property?
- There is no obvious way to, across any page in the whole system, go "You know what? Screw you and what you want, I want to override the entire contents of this page and you can't stop me!" I was excited earlier when I thought that all pages would end up with a $page['content']['#markup'] property, which would let me do things like this:

if ($some_condition) {
  // Override the contents of this page.
  $page['content']['#markup'] = theme('my_swanky_output', $page);
}

But it looks like I'll need to do a bunch of checking of whether or not property A exists in the $page, and if so, then override property B, but if property C is set, then override property D instead, etc. This is the kind of thing that's going to take me an hour and lots of flow-charting and pointing and gesturing to explain to a room full of new Drupal developers, and it's only going to get worse once this patch is committed and we start converting more pages over, each of which add their own funky properties.

We need consistency, consistency, consistency. :)

@Frando:

Re: #2, Sure, I'm comfortable leaving the other stuff to a follow-up patch.

Re: #4, I have the sneaking suspicion that once this is all over, we'll end up with far more than two small #properties that are $page array-specific. ;) Just trying to prepare for the future in advance. #111079: Render API: Differentiate internal from user-settable renderable array attributes wouldn't actually help us much here, because things like #attributes and #required are not (normally) internal properties for things that get passed to drupal_render().

Webchick, I think it might be unrealistic to expect the stuff inside of $page['content'] to be consistent across all pages. The content itself will be different from one page to another, and the key is that they use the same representational structure that other drupal_render()able structures do.

If there are places where standards make sense (like $form['buttons']) we can implement them, but I don't think there is much to be gained from trying to plan out a "consistent" structure when we have never even had any structure at all up to this point.

Also, regarding the difference between #properties and sub-elements, that's a pretty well established issue in the drupal_render() family. #properties are settings and flags that are used to control/manage the rendering of an element itself, while children are the 'meat' of a container element. I'm nervous about putting big "bin" #properties on elements, as they lead to easy bloat and move us farther away from OOP-like child/property mappings.

Every renderable element implements custom #properties to do its work; I think the biggest danger is in littering the system with magical 'global' #properties that are undocumented but change the behavior of anything they're attached to...

That said, I agree that putting things like the help messages, status messages, warnings, etc in the page array would be a big improvement. Right now they're treated very differently, though, and I'm concerned that this will turn into a boil-the-ocean patch rather than an "Enabling Improvement".

The Developer Experience is -- repeat -- no different than it is currently after this patch is installed. It's just another hook point, and another option for modules returning data. hook_page_alter(), even if we don't move everything to it in this version of the patch, is still a huge improvement over preg_replace_all(). ;-)

Bit of profiling.

With 90 nodes on a front page, calls to drupal_render() go up from 690 to 792.

Benchmarks are fairly equivalent, few ticks slower with the patch but not significant.

HEAD:
/node
2.75 reqs/sec

/node/n
12.58

Patch:
/node
2.69 reqs/sec

/node/n
12.56
So a little slower, but not too much.

node/n profile with the patch applied for comparison.

Summary of a half hour or so discussion about the consistency concerns I have...

This was coming from a place where we have certain... how you say.... "ball of writhing, venomous snakes" places in Drupal where if modules aren't playing nicely together, the world becomes a painful, miserable, and awful place. Two such places that immediately spring to mind are hook_nodeapi (two modules adding a $node->parent property... everyone loses!) and .info files' "package" property (User interface vs. UI vs. Interface vs. ....).

However, Eaton argued convincingly that the situation with the $page object is more akin to how people choose to name their $form elements in a form definition function. You can name it $form['parent'] or $form['book_parent'] or $form['wookie'] or whatever you want; it doesn't ultimately matter. This is because each form is ultimately "owned" by a specific module, just as a given page on the site is. And, like forms, it would be the responsibility of other modules that want to alter it to play nicely with whatever elements the "mother" page was outputting.

My concerns with consistency can probably be summarized as:

"As we convert more pages over once this patch is committed, let's make a concerted effort to look for patterns that make sense to standardize on."

So, I'm comfortable with these concerns not holding this patch up now, as long as we agree to look for these patterns in future patches. :)

Since we're leaving messages/help/titles/etc. for a follow-up patch, and since we don't yet have enough data to know what's best to standardize on in terms of the page #properties, that basically leaves only the documentation improvements. I also want to make one last pass at the code again now that I understand better what the patch does.

Oh, and here's a summary of the changes introduced in this patch. It's not exhaustive, but should give people an idea:

All pages:
$page['header']
$page['left']
$page['right']
$page['footer']
$page['content']

All blocks:
$page[$region]['#block'] = (block object)

Legacy pages:
$page['content']['main']['#markup'] = (HTML markup of page)

Term listing page:
$page['content']['term_description']['#markup'] = (description of term)
$page['content']['nodes'][$nid]['body']['#markup'] = (node body)
$page['content']['nodes'][$nid]['links'] = (array of blog, comment, terms, etc. links)
$page['content']['nodes'][$nid]['#node'] = (node object)
$page['content']['no_posts']['#markup'] = (message about there being no posts)
$page['content']['pager']['#markup'] = (Pager HTML)

Node listing page:
$page['content']['nodes'][$nid]['body']['#markup'] = (node body)
$page['content']['nodes'][$nid]['links'] = (array of blog, comment, terms, etc. links)
$page['content']['nodes'][$nid]['#node'] = (node object)
$page['content']['pager']['#markup'] = (Pager HTML)

Single node page:
$page['content']['body']['#markup'] = (node body)
$page['content']['links'] = (array of blog, comment, terms, etc. links)
$page['content']['comments']['#markup'] = (html output of comments)
$page['content']['files']['#markup'] = (html output of file uploads)
$page['content']['#node'] = (node object)

(summary of the review leading to patch above)

Now for the nitpick pass!

- + *     '#type': Value is always 'page'. This pushes the themeing through page.tpl.php.

themeing -> theming. (I even grepped!)

+ *   An array node IDs.

missing an 'of'

+  $page['content'] = is_array($content) ? $content : array('main' => array('#markup' => $content));

Sun brought up that 'main' isn't a construct we ever use in Drupal. Eaton suggested 'default' instead, which implies "nothing has provided any renderable chunks here so you're just getting default output." Sounds good to me.

+    $build += node_build_multiple($nodes);

We provide a node_build_multiple() without a corresponding node_build(). This will be confusing for new people since all of the other X_Y_multiple() functions have a singular one as well. Moshe pointed out that the equivalent of a singular version is node_view(), so we should rename this to node_build() (extra side-bonus, no more confusion between node_show() and node_view(). Hooray!)

We also spent some time talking about implications on the theming side; for example, theme_taxonomy_term_page() was removed, and there are a few wrapper divs that are now moved beyond the reach of theme_X functions as a result of this patch. In future patches, we might want to explore alternative ways to make overriding these available to people who are scared of breaking into module development.

Subscribing (with much excitement)

How does this compare to #134478: Refactor page (and node) rendering?

This patch is a) about to be ready to go in comparison to #134478 and b) is a very important first step to allow rendering content into different output formats (by keeping output format independent array structures up to the very last point)

Reroll after a failed hunk.
Only one little change to #109: If we set the #theme property on the page element, and leave #type empty, we can just use drupal_render($page) in drupal_render_page instead of theme('page'), which makes the theme function used overrideable in hook_page_alter(). This tiny changes would allow modules to use another theme function for page rendering, e.g. to print content into different output formats. And no drawbacks at all.

I really think that this one is ready to go in. A lot of great stuff can happen afterwards. Now we just need someone to finally RTBC it.

It looks like we're getting rid of theme_page. If we are, should we remove the function? Now that modules can use hook_page_alter() to change the theme, how do themes alter the page since theme('page') isn't called.

It also looks like drupal_render_page prints its contents within the function, but when it's called it's called with print drupal_render_page(). It would make more sense to return from drupal_render_page() and print from there.

"Backward compatibility - allow menu callbacks to return strings."....... Not sure how I feel about that one. Would it be better to stick the ability to render strings inside the PHP Doc?

Could we get some tests?

It would make sense to give the theme the final change to page_alter(), though that clearly wouldn't be through the hook system.

No, we're not at all getting rid of theme_page. theme_page still themes the page, after all. We just made it possible for contributed modules to switch to another theme function. By default and in core, the page is still themed by theme_page (preprocess_page and page.tpl.php, that is). And altering happens before the theming, so not sure what you mean there.

@David: Themes can of course still override theme_page through the regular theme override system. So the theme still has the "final control". No change here at all.

I changed drupal_render_page to return the rendered page instead of printing it directly. Makes sense.
Also removed the "backward compability" part of that comment - there certainly are situations where it's just fine for menu callbacks to return strings.

Tests - well, what should be tested? If something breaks during page rendering, pretty much all tests will break. And hook_page_alter should already be covered by block tests.

Here's a re-roll with new docs for hook_page_alter() that Eaton, Moshe, Crell, and I worked on tonight. No other changes.

And mine too.

me too :-)

Fixed a bunch of PHPDoc and coding-style issues.

Made a few small code changes, but i'd like to run this through the testing bot.

It passed.

Fixed indentation in improved PHPDoc blocks.

I'm biting my nails in anticipation.

This patch is RTBC but failed to apply. There is a chunk failing in taxonomy module -- needs a quick reroll! :)

This should work.

Committed to CVS HEAD! Please update the upgrade instructions before marking it 'fixed'. Thanks for all the hard work and the precious reviews.

I really hope someone can write this up... I'd noticed that this broke the devel module's display redirection page function. I started to roll a patch and consulted the update docs but there weren't any.

I think this patch broke the Chameleon theme (by removing the theme_blocks-function.) I've opened an issue for this (#374650: Chameleon broken by hook_page_alter).