Jump to:
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | DrupalWTF, DX (Developer Experience), Favorite-of-Dries |
Issue Summary
After I started to use the API I suggested and got committed at #428744: Make the main page content a real block, I realized that the API is not too useful for page callback handling. The current D7 expectation is that page callbacks either return the main page content or set that via a function and return a page structure array. Since they can also return other arrays, one needs to check for the exact contents of the array even to tell what exactly was returned. This is the code I needed to use to tell what a menu callback returned in a contrib D7 module:
<?php
if (is_array($content)) {
if (!isset($page['#type']) || ($page['#type'] != 'page')) {
// Not a page structure array, but some other array.
$content = drupal_render($content);
}
else {
// Was a page structure array. Get actual
// page contents and use/render that.
$content = drupal_set_page_content();
if (is_array($content)) {
$content = drupal_render($content);
}
}
}
?>This is the code required to get a string out of the output of a menu callback. Not good.
Instead I suggest that we always request menu callbacks to return a string or array and the array would be a drupal_render()-able structure (not a page structure). The page attributes would be set with drupal_set_page_attributes(). The above contrib code would simplify to:
<?php
if (is_array($content)) {
$content = drupal_render($content);
}
?>Simpler, eh? :) Also, makes core code in system_batch_page() and drupal_not_found() much simpler.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| dx-page-return.patch | 4.03 KB | Idle | Failed: 11361 passes, 3 fails, 22 exceptions | View details | Re-test |
Comments
#1
BTW I've tested installation batches and 404 pages and all worked fine for me.
#2
The last submitted patch failed testing.
#3
This is what we were trying to avoid in #351235-44: hook_page_alter(). Back then we called it drupal_decorate_page(). We did not want the page generation to have side effects as that complicates caching and testing. I need david strauss to give a fuller explanation. I will point him here.
#4
The page generators have their side effect possibility anyway, this patch just exposes that in a different API. hook_page_alter() gets all the same structure anyway, it is not at all affected. By the time the data gets to hook_page_alter() it is all the same it was before this patch.
#5
When working on the original hook_page_alter() patch, I introduced a similar method to set page properties. I called it drupal_decorate_page(), but it was basically the same. See #351235-43: hook_page_alter() and the following comments. For a reasoning why it was rejected there see #351235-79: hook_page_alter().
Maybe an alternative would be to pass $page to all menu callbacks as the first parameter, and make them all set whatever they want. This would require to change all menu callbacks, though.
#6
I fail to the see the problem here.
1. Before the patch, the page callback either returns a string or an array of some sort.
2. Then that array might be a page structure array or some other renderable array.
3. If it is a string or a renderable array, Drupal builds up the default page structure array and uses that.
4. If it is a page array, it requests the page content from drupal_set_content().
So page callbacks can return 3 different things: (a) main content string (b) main content array (c) page structure array (which is just to set two known properties).
How you get rendered content is different in each case, in (a) you get it directly, in (b) you call drupal_render() on it, with (c) you need to first get the array from drupal_set_content() and render that.
I fail to see why it is bad idea to be consistent about the return value of page callbacks, and get them return content strings or arrays only. They can influence the page attributes either way. If it is through a return value or a function call, it is still the same. Would the point be that others should not be allowed to set page attributes, so we should not have a function which non-page callback functions might call? In that case, to be consistent with page callbacks, we should enforce them calling drupal_set_content() in all cases and not return any content info but a page structure array in all cases. That would be a total turn from where Drupal 6 was, so I am not sure why would that be better.
#7
drupal_set_page_content()is absolutely a bad thing, and I agree that menu callbacks should always return their content, not "set" it into a static variable.Right now, I'm trying to wrap my head around the overall block/content vision. I've long advocated (though not very vocally) that "content" should be a block. I'm glad to see that #428744 made that happen. What I'm trying to reconcile now is the combination of (1) blocks with path-based visibility rules and (2) content returned from menu page callbacks.
They now both display content in blocks based on path matches. The only difference is that blocks are generally assumed to display on all (or many) paths as a starting point, and content is generally assumed to be bound to a single path. That leaves developers with two overlapping APIs distinguished by little more than best practices.
The real confusion comes with decorating existing (say, node) content. Do I now (1) alter the existing page content array or (2) add a block whose content changes based on my own reading of the path? The former makes my addition manipulable by other alter hooks and the theme layer. The latter allows users to move my decoration around as a block.
Now that content is a block, I'd really like to unify the block and menu/page content systems. Is there a way to do this without making every menu item a path-restricted block? Could menu/page content and blocks be unified into a hierarchical (drill-down) page layout UI? This unification has been in the cards for years, with tools like Views allowing output into pages or blocks, other modules designed to offer things like "nodes as blocks," and Panels being used to break down $content into a block-ish layout that allows inclusion of nodes, blocks, etc.
I'm not trying to derail this issue. I just think it's critical to understand where page content stands in the overall design before attempting to refine how we handle it.
#8
@Gábor Moshe is actually agreeing with you. My first reading of his comment also got the mistaken impression that your patch was "the problem" to be avoided. I believe Moshe actually means the problem this issue is trying to solve is the same design we were trying to avoid (and I was also arguing against) in the other issue.
#9
subscribing.
#10
David: thanks for enlightening me on Moshe's comment. I thought he says that this patch does not make sense.
In this issue, I am just trying to clean up the mess my patch created :)
On the bigger (and offtopic) question of page layout, my belief is that we can make all of those blocks: such that themes will not have *special* support and settings checkboxes for primary links and the search box and the like, but they could style them differently when they are put in certain regions (such as when a primary menu is put into the header, it will show up in a horizontal way, mimicking the special primary menu there was in D6, but you can move that block to the sidebar to get a vertical view). That would eliminate most special casing of stuff for themes so themers can just concentrate on styling with CSS selectors. Less PHP munging, more CSS munging. I am bugged by the duplication of things like the primary/secondary menus and the search box in the theme settings and the blocks system. Once we get these all under one roof, we can use the blocks system to position stuff, set path limitations, etc. (Drupal 7 already does this for help text, highlighted content (was site mission), etc). That said, with every step on the way, the core blocks system would look more like panels, in that you can disable/override most of a page output. Of course it would not be like Panels in that it would not offer versatile layout containers and other flexible stuff, but it would be an admin UI for page organization much more like Panels is. I hope this is on the path for Panels in core, which will most probably not end in D7.
#11
@Gábor All good points. We probably should just move forward for now on your proposal. I'll support making page callbacks standard renderable arrays, especially if we start embedding CSS and JS includes into these arrays instead of calling side-effect functions. Part of the page array design, at least initially, was to minimize page callback side-effects.
I'm not sure how to solve the title-setting problem, though.
drupal_set_title()is one of those nasty side-effect functions, and part of the original page array proposal was that the page title would be an (alterable) part of this array. Unlike CSS and JS, there can only be one title on a page, and it just doesn't make sense to possibly override this title every time a content array is rendered.Again, I'm going somewhat beyond the intention of your work on this issue, but I'd like to minimize the number of page callback return formats, even during development.
#12
Tagging!
#13
More tags... when I read http://drupal.org/node/224333#theme_page I had a serious "WTF kind of API is that?" moment. ;) Glad to have found this issue about cleaning up the mess, and I totally support David's #7 and Gabor's #10. Also subscribing...