At #378916: Split content region into content_above and content_below it was suggested that we should decouple the main page content and the content region, and make the content region a "content_top" and "content_bottom" region, which would show above and below the content by default. My response to that was that regions are made for ordering, so if we are looking at a "top" and "bottom" region, we should rather have one region instead and make the item in question movable as well. Hence the patch to make the content movable inside its own region.

I proposed this on comment #39 in that issue: http://drupal.org/node/378916#comment-1442974 and was asked to open a new issue and mark that postponed on this one (if this gets committed, that gets closed).


Here is a patch which is tested to work with Garland and Stark (and assumed to work with Minnelli obviously). I've also tested basic update.php screens (since that is using the maintenance theme), and that also worked.

What I do here is basically at any point someone tries to set the page, I store the page data for a system module block to use (the function used to generate that is named "drupal_get_page()" which both does some metadata generation and renderable content array generation).

The system module block can be positioned however you want it to and stuff can be put above and below it. Obviously if you disable this block, you can easily make damage to your experience, and getting it working again would require digging in the DB. *However* this opens the door to disabling this and only using other block regions on some pages allowing for more flexible layouts when you need to arrange your page in an unconventional way.

For testers: either install your Drupal 7 fresh or uncomment the line where it says UNCOMMENT. Then do what the comment says (enable and position the content block) and comment it out again.

I think having one region where you can order stuff is much more useful then having "top" and "bottom" regions, especially when in certain theming it could be shown left and right to the content region (eg. horizontal scrolling layouts), so it would rather be "before" and "after", but moving stuff from region to region just to work around something you cannot move is IMHO worse then just making what you cannot move movable :)

This is how it looks on the blocks admin (image uploaded on the above mentioned issue):

Ps. I already have issues to make the help text a block and plan to follow along the line given enough progress in how this gets implemented. :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

There is some support and some concerns in the issue. Reciting some concerns:

- The content region and main page content block can be renamed obviously to whatever fits better. Jeff Noyes says "The word content is too overloaded in Drupal, and I think "body" or "page body" is what comes to mind when you think of the main element of a page."
- This block can be disabled. That would make use of the site rather difficult. Sun says we can easily add a validation handler to the form and ensure that the page body is always positioned in a region. I agree.
- New/additional themes take their config from the default theme. Blocks are replicated in the new theme in the same regions as the default one. catch says we can require a "content" or "body" region in all themes. I agree.
- Block module is not required anymore. The idea is that alternate modules implemented to manage pages can take its place, like Panels. Panels exposes all blocks, so it would expose this block just like the others. Also, it can work with the utility function to get the page content, while a proper panel is not set up to show the page.

Any other concerns I did not copy over or did not yet see?

catch’s picture

I really love this idea, no new concerns / ideas that weren't already posted on the other issue.

Frando’s picture

In general this patch sounds like a good idea.
With the page rendering patch that got in earlier page rendering already got much much more flexible, which means block.module is in no way special anymore - any module can easily add content to the regions, and block.module itself also just adds its content via hook_page_alter(). The point that makes this patch still worthwhile IMO is that block.module is currently the only module providing an UI for moving content between the regions, and therefore is UI-wise certainly the easiest way available currently.

A few remarks:
* in function system_set_content_block, please do not invoke drupal_render(), just return the renderable array. This keeps the main page content alterable in hook_page_alter(). It will be rendered later on anyways.
* We might want to give block.module a lower weight, now that all page content is added via block module's hook_page_alter() implementation and not earlier. This makes hook_page_alter() quite useless for modules that come earlier than block.module.
* We will need an update path, which should be easy though (just enabling the new block).

mfer’s picture

Issue tags: +Needs design review
Zarabadoo’s picture

I really like this idea. This allows us to make some very generalized themes and eliminate some of the region bloat. This would allow for us themers to simply add one region for each place we expect content to go. If and end user wants the main content in the sidebar, so be it.

I have seen this in action, and it has my full support.

Jacine’s picture

I was a little concerned about the need for this at first, but after reading through #378916: Split content region into content_above and content_below, I'm on board because it definitely increases flexibility, which I am all for.

It definitely has the potential to add a LOT of work for contrib theme maintainers. I foresee a flood of support and feature requests because users are trying to add content to regions that styles weren't designed for, or that output is too wide for, etc. I think this will probably increase the demand for more flexible layouts.

However, I don't think any of that is necessarily a bad thing.

If possible though, as a themer I would like to be able to restrict which regions the main content can be placed in. For example: Adding content to sidebar = fine. Adding content to the header or footer = No, thanks. Maybe this could be done in the .info file?

restrictions[content] = left, right, maintop, mainmiddle, mainbottom

moshe weitzman’s picture

Looks good. I second the feedback of Frando.

JohnAlbin’s picture

As much as I liked my own patch in #378916: Split content region into content_above and content_below, I like this solution even better.

cburschka’s picture

Status: Needs review » Needs work

Remember that block.module is now optional.

If we make the main content depend on block.module, then we have two options:

1.) Turn this:

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

into this:

if (module_exists('block')) {
  system_set_content_block($content);
}
else {
  $page['content'] = is_array($content) ? $content : array('main' => array('#markup' => $content));
}

This special condition isn't a bad thing necessarily: We have other places where a core system (menu.inc) is required, but the ability to customize it (menu.module) is optional. In this case, the content is displayed normally without block.module, but as a block if block.module is enabled.

OR:

2. Set block.module back to required - it makes no sense to have something marked optional if the entire page structure depends on it being enabled.

Gábor Hojtsy’s picture

Arancaytar: I tried to cover this in #1, but seems like that slipped through your attention. So far, people who said that block module is optional said it is to run with a replacement not to run bare bones without block module and without a replacement even. If we consider there will be a replacement, this is already addressed above. Panels as a replacement exposes blocks, so this block will be exposed. It is then up to panels module to handle this situation IMHO. Having content possibly as a region and also as a non-region (if panels are not used) might result awkward special casing for themes.

catch’s picture

The main question there is whether there's any kind of fallback/recovery when you disable block module through the UI and don't have panels enabled. It'll be very easy for people to lock themselves out completely.

Gábor Hojtsy’s picture

Well, that part is up for discussion, and while the patch needs work in that regard, it is not gonna be worked on until we settle on the conceptual idea. We already discussed forcing the content block to be in one region on the blocks UI. We can have an extra confirmation step on the modules screen when we see someone turning off the blocks module for example, reminding that they are gonna break the site unless they have a replacement. We can also just do what Arancaytar says. Or someone might have an even better idea :) Opinions?

tstoeckler’s picture

+1 for #13.
With all those used-to-be-required modules going optional, I think an extra step of validation is quite handy. Something that keeps unexperienced users from killing their site, and just one more fast click for crazy developers who turn off block.module.
With node module potentially being optional such a warning message could become the rule for turning off such monumental modules.

moshe weitzman’s picture

Right now a module can only be required or not. We could expand to required or recommended (this one is new) or optional. But enough about that - lets work on the substance of this patch.

Frando’s picture

FileSize
9.11 KB

Worked a bit on the patch. system_set_content_block() now uses drupal_static and keeps the main content as an renderable array. I modified the block theme preprocess function accordingly (to use drupal_render). I also added an update function. This adds the block to garland only, though, not sure what happens if another theme is used (presumably you'd be locked out of your site. Another point to give more thinking).

Now, what I don't really like is the new way the page rendering array is structured. If you use hook_page_alter e.g. on node/1, the node's body is now in $page['content']['blocks']['system_main']['#block']->content['nodes'][1]['body'], opposed to $page['content']['nodes'][1]['body'] before the patch. Especially I don't like that the main page content is hidden in the #block object to all-arrays before. Not really sure yet what to do about that, but when working on the page rendering patch, I was thinking of possibilites to use the final page array to return a page in different output formats, even in an all-contrib solution. This doesn't really hinder this but it is less nice. I don't have a good solution so far, though, still thinking about it. We could for example skip the ['blocks'] part now that everything is in there. We could also expose #block['content'] as the top level array part, and make the #block object a property of the block's content.

Gábor Hojtsy’s picture

@Frando: good thinking. I'd save far reaching array restructurings here though. Let's just make this a block and then restructure the array in a follow up patch. Otherwise this is not gonna get into Drupal 7 I fear.

SeanBannister’s picture

Sub

Gábor Hojtsy’s picture

@Frando: Reviewed your patch. Your changes look good, except that I would not merge in any changes to simplify the block theming. Let's do that elsewhere, it has nothing to do with this improvement and discussion of that just derails this issue. Let's remove that part of the patch, please.

Dries’s picture

Issue tags: +Favorite-of-Dries

I'm a fan of this patch. Adding it to my favorites.

I agree that we should leave the theme processing out of this patch -- personally, I'm not a big fan of those is_array() checks:
$variables['content'] = is_array($block->content) ? drupal_render($block->content) : $block->content;

I agree with the solutions outlined in #1. If block module can treat the content block special, Panels should also be able to give it special treatment.

Frando’s picture

Status: Needs work » Needs review
FileSize
6.26 KB

New patch attached. Removed the block theming preprocess changes.

The is_array checks are still there. Currently, we block content can now either be a string or a renderable array. The proper way to fix this would be to restructure the way block viewing works (near the theme layer, there should always be a renderable array). This would require some more changes, though, which I would really like to avoid in this patch.

I think this patch is now as small as it can be.

Still thinking about the update function - as the patch stands now, the block is enabled for the garland theme. But what happens if someone uses a different theme? He would find his site unusable, because the main content block wouldn't be enabled automatically when updating. The problem is - as region names can currently be chosen by the theme completely randomly, how can we enable the block if we don't know the name of the main content region?

Two ideas come to my mind, we could add a property to the .info files of themes that tag one region as the "default region for the main content", or make it required for all themes to have a region called "content".

catch’s picture

I'd go for a required region called 'content' - we have a required variable called $content now so it's not too big a change. It also means it'll be consistent in the blocks admin between different themes.

Also seems there's nothing in this patch to prevent users disabling the block region and rendering their site impossible to use - just some validation or something would be enough.

moshe weitzman’s picture

Agreed - make it required for all themes to have a region called "content". Consistentcy here is very useful for AJAX replacement of content.

Gábor Hojtsy’s picture

Worked forward from Frando's patch to include safety nets and clean up some core. Attached patch includes the following changes:

- Remove manual testing helper comment and code to avoid cruft.
- Converted multiline is_array() ternary to an if()
- Safety net 1: validate blocks ordering page submission and do not accept form if the content block would have been disabled (see remaining problem below)
- Safety net 2: make it a core incompatibility problem if a theme does not have a content region, so such themes cannot be enabled in Drupal 7
- Safety net 3 (AKA better upgrade path): add the new block to all themes (enabled/disabled) which have at least one block; other themes will be taken care of runtime with block_initialize_theme_blocks() when enabled, and we should not interfere with that business here

The only problem I see here is that when the form validator finds out I am going to disable the content block, the block comes up in the content region, but the dropdown has the wrong value. I tried to fix the dropdown value with setting the suggested region properly in the validate function and then attempting to use the form_state values in the form construction instead of the values passed in based on database data. I assume this might fail because of the form cache, but would generally welcome some help in fixing this. This is how it looks:

Gábor Hojtsy’s picture

Updated to use update_sql() instead of db_query() in the update function, so we get feedback in the update SQL list on what happened. No other change.

Dries’s picture

1. Have you tried setting $form_state['rebuild'] = TRUE (normally for multi-step forms) or $form['#no_cache'] = TRUE?

2. + // Use form submission data to show values if showing the form again. -- this was not 100% clear to me. Why is that new? Because the submission can actually fail now?

An easier solution might be to form_alter out the "no region" option but I guess that doesn't work with the drag-and-drop. It would, however, prevent misconfiguration to begin with.

Gábor Hojtsy’s picture

1. Will look into that.
2. Yes, previously the form only took data from the database, since the data was saved in all cases. Now it should also work with data coming in from an invalid form submission.
3. My gut reaction to altering out that option is that after drag and drop, you'll get an "Invalid selection" type of error message, which is not gonna help you in why it is a problem. But going to look into that too.

Berdir’s picture

Status: Needs review » Needs work

All new patches should use DBTNG syntax for things they introduce or change..

+ if (db_result(db_query("SELECT COUNT(*) FROM {block} WHERE theme = '%s'", $theme->name))) {
db_query()->fetchField(), with the correct placeholder syntax (:theme): http://drupal.org/node/310072

+ $ret[] = update_sql("INSERT INTO {block} (module, delta, theme, status, weight, region, pages, cache) VALUES ('system', 'main', '" . $theme->name . "', 1, 0, 'content', '', -1)");
And that should use db_insert(): http://drupal.org/node/310079

+  // Enable 5 standard blocks.
+  db_query("INSERT INTO {block} (module, delta, theme, status, weight, region, pages, cache) VALUES ('%s', '%s', '%s', %d, %d, '%s', '%s', %d)", 'system', 'main', 'garland', 1, 0, 'content', '', -1);
...

+  // Enable 4 standard blocks.
+  db_query("INSERT INTO {block} (module, delta, theme, status, weight, region, pages, cache) VALUES ('%s', '%s', '%s', %d, %d, '%s', '%s', %d)", 'system', 'main', 'garland', 1, 0, 'content', '', -1);

You could probably also convert the other insert's there and use multi-insert to save a few queries.. :)

Anonymous’s picture

Next we'll have blocks within blocks. ;p

The error for <none>; does the same happen for moving "Main page content" to any other region than the Content region? Should it not be fixed to one region; i.e. immovable?

Gábor Hojtsy’s picture

I am not sure it should be fixed to one region.

Anonymous’s picture

I can envision why you would say that but I think there needs to be other considerations before something useful can be determined. Maybe a theme change only though. What about the sticky nodes? Should a block be created just for them? A two column site may have sticky on the left and typical stories on the right, etc. Is one Content block enough? I can see Content top, left, center, right and bottom.

Gábor Hojtsy’s picture

@earnie: The content region already allows you to put stuff above or below the content block, so having top and bottom is solved right there. Having right and left is solved with the regular left and right sidebars to some degree, while some themes have inline regions which float in the content area. Since Drupal core does not have those features, we should not cover that here. If you'd want to see such features, I'd suggest opening a new issue for that.

webchick’s picture

subscribe. Somehow this was off my radar. Awesome stuff! Will try and review a bit later.

Anonymous’s picture

@Gábor Hojtsy: I'm bringing forth discussion of a requested feature and Header/Footer is not the same as Content top/bottom so it isn't solved. Yes, contributed themes may implement this, I've seen it already but we're discussing core ability from the start. And if we're making this change why not allow for regionalizing the main page content?

dvessel’s picture

subscribing. I'm really liking this.

Berdir’s picture

@earnie
Content is now one block *inside* the main content region. So you can easily place another blocks in the same region and then configure the order in the block administration page. Having single blocks for sticky nodes etc. is a completely different feature and you could do that with Views.

tstoeckler’s picture

@ #31 Since there is currently ONE $content variable this issue is also about turning this variable into ONE region. The idea is to let you order everything (including content) with the powerful blocks system. To do what you proposed simply get Views to grab all sticky nodes put them in a block and put them next to (on the top, on the bottom, on the left, on the right...) content.

@ #29 That would mean that if you have a theme that has two regions that look somewhat like the main page content (think two regions next to each other in the main page area, one e.g. for the node/add form on the left and one for extensive help on the right) you could not swap content to either region but have to fix it to one. I wouldn't immediately give up all the flexibility you get by making this a block.

EDIT: Holy cow, crosspost with 5 people! The world turns so fast these days...

Gábor Hojtsy’s picture

Status: Needs work » Postponed

Berdir: ok, I am posting the DBTNG-ificated version of #240873: Move custom help settings to blocks first, since it affects similar code areas and is less of a target for debates (almost committed). Then we can get back here and DBTNG-ificate this as well.

int’s picture

Status: Postponed » Needs work
Gábor Hojtsy’s picture

Given that we will keep modifying the same update function to achieve an efficient upgrade path and to avoid copy-pasting the same code multitude times, I am trying to get http://drupal.org/node/428800#comment-1550776 in soon and then coming back to this patch, since this one seems to have more debate around it :)

catch’s picture

Status: Needs work » Postponed

Let's mark this postponed on that one.

Gábor Hojtsy’s picture

Status: Postponed » Needs work
FileSize
9.14 KB

I figured I should reroll this anyway, since there were at least three conflicting files and we still have a lingering FAPI issue to solve:

- rerolled for the two conflicts in .profile files given the recent changes in DBNTG-ification of the block creation in the profiles
- rerolled for the move of the block preprocess from theme.inc to block.module
- rerolled for the arrival of the wrapper code of the help block, so we can just do the upgrade path of this patch in one new line plus one line of comment :)

This should resolve Berdir's concern of not being DBTNG compliant. The only remaining issue now then is that the redisplayed form after validation places the content block into the content region, but the dropdown is not updated. Described in detail in #24.

I believe we can keep this at "needs work", since this can be independently applied to Drupal 7 HEAD and tested + fixed.

moshe weitzman’s picture

FileSize
8.63 KB

Bah - lost my longer reply.

Great patch.

Attached adds update function and block_install() feature where we set system.weight = -5 for block module so that others can easily make changes during hook_page_alter().

In a later patch, I will fix up the horrid obfucation that this patch adds to the $page array. See Frando's comment in #16.

xmacinfo’s picture

Great patch!

But please, do not enforce the usage of the "Main page content" block.

There are times we do not want to output the "content" part in a page template. In fact, I have a few clients that asked for this to which I had to create a special no-content page-front.tpl.php template.

Forcing content to appear will probably break my templates and will not help my clients.

I could still hide the content with CSS, though, but this is not a clean solution.

Gábor Hojtsy’s picture

@xmacinfo: the idea is that you would not need to hide the "Main page content" block on each page of the site. To just remove it on certain pages, you can use the block visibility settings, just like with any other blocks. No need for a special template or CSS hacks. Even easier then in Drupal 6, right? :)

xmacinfo’s picture

That's a nice way to put it and it reassures me.

You are right, as you explain it, it'll be a lot easier in D7.

wretched sinner - saved by grace’s picture

@earnie (#31)

I have opened a separate issue (#453744: Create a block for "Sticky" nodes) for splitting out the sticky nodes from the Content block. Let's not derail this conversation here!

ckng’s picture

subscribe.
Like the idea, make it simpler for users to just learn block and maybe block layout tool in the future.

Gábor Hojtsy’s picture

@Moshe: thanks for working on the patch. IMHO good changes. The form problem still applies, but otherwise should IMHO be good.

Gábor Hojtsy’s picture

FileSize
10.14 KB

Given recent commits around areas touched by this patch, there were some rejects and fuzz when applying. Here is a reroll for your testing pleasure. Still needs to the FAPI problem fixed.

Jaza’s picture

Subscribe

chx’s picture

Gabor, the fix you look for is #382634: Previewing multiselect taxonomy fields lose value on multistep node forms

Edit: that issue might need to be simply wont fixed and the form.inc fix rolled in this patch -- surely its easier to test here. And though Eaton noted two weeks ago that issue is fixed, i still think you are biten by the same bug.

Gábor Hojtsy’s picture

Unfortunately something committed in the past week broke this patch pretty badly. I suspect #355236: Refactor drupal_render theming - docs because that directly changed how rendering works and even changed the drupal_get_page() function we are altering here. Also, the current looks of Drupal with this patch is pretty amusing. Looks like the page is rendered again in all its glory in the content area again in some cases.

Managed to track this down to places where Drupal returns the page content from a page callback with the drupal_get_page() wrapper. So not all pages are broken, but quite many are. This makes this patch a pretty big needs work (or the patch which was committed and broke this behavior).

I am attaching an actual roll of the patch with a changelog entry, so people can help pick this up. I'd greatly appreciate some debugging help around here, since I am not too familiar with the rendering inner workings.

Gábor Hojtsy’s picture

FileSize
25.45 KB

Ehm, so this is how it looks now:

Gábor Hojtsy’s picture

FileSize
12.67 KB

Ugh, found the problem and fixed it. The real issue which was exposed by recent changes but was there already was that drupal_render_page() assumed that there would be a 'content' item in the page array received, and if not, it assumed it was not a renderable page array but some other renderable array.

I've replaced this with more robust checking now in drupal_render_page(), so that it checks for an array with a #type key with a value equaling 'page'. If there is a given array but it does not conform (such as when you use drupal_get_form() as your page callback), it is treated like it is not a page array and is passed to drupal_get_page() for rendering. Due to the faulty check, all pages failed with the previous patch which were using arrays, since none of them had the 'content' key so were rerendered.

Also found a notice in block module around #show_blocks checking, which was fixed. BTW noted that #show_blocks is misnamed since it only hides the sidebars (left/right), and given that the main page content will also be a block, it will be even more misleading. I think this should be fixed in follow up issues.

Attached patch now works again, but still lacks a solution for the Form API problem. I've looked at the solution suggested by @chx at #52, but turns out that change is already in Drupal 7, and this patch still does not work right regarding the select box behavior.

Frando’s picture

Hm, #355236: Refactor drupal_render theming - docs was committed on February 3, so even before this patch was started. I don't think it has anything to do with this issue.

moshe weitzman’s picture

I can confirm that the drupal_render_page change that was just introduced is very helpful and proper. Sorry for my brittle check there.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
13.53 KB

Instead of sweating over the Form API problem, I've looked into this suggestion by Dries:

An easier solution might be to form_alter out the "no region" option but I guess that doesn't work with the drag-and-drop. It would, however, prevent misconfiguration to begin with.

So what I did is that I've removed this region for the system block directly in block module (IMHO no reason to form_alter in system.module, since we know this is gonna be done anyway). This already prevents that a user submits a "no region" selection for the item. Then I went into the drag and drop handler, and whenever a drop is handled, I've looked into the dropzone's name and searched for it in the list of select options. If the zone name is not in the select options, then an alert() is popped up which informs the user that the given block cannot be dropped in the given region. Finally, right after the alert box is closed, the block is moved back to where it was previously (that is before the user moved it into the forbidden region).

So one could move the block to the Left sidebar, and before saving the page try to move it to the Disabled region, the alert() would pop up and the block would be moved back to the Left sidebar (since that is where the user moved it originally, but not yet saved). The yellow mark saying that saving the form is awaited will be shown on the row still obviously.

I am not certain that the alert() is the best way to handle it, but we need to tell people why we force this stuff out of the disabled region. Any better ways we have?

yched’s picture

FWIW, I think that overriding the Drupal.tableDrag.prototype.row.prototype.validIndentInterval() function to return 'min' > 'max', you could prevent the positions below 'no region' to appear as valid drop targets while dragging the 'content' row. Prevents the drop to begin with, but then I guess it doesn't let you inform the user about why the move is not allowed.

[edit : scratch that, it appears it probably would not catch keyboard-driven d-n-d, while the proposed solution in #58 will]

tstoeckler’s picture

Tested the patch. Upgrade works nicely. On Garland you don't even notice the magic that has happened, which is a good thing, as not to scare users that just wan't their site to behave as it did. (It does!)
The Blocks admin form works perfectly as well. When trying to disable "Main page content" via Drag and Drop, I get a small popup saying that I am not allowed to place that block in that region. Which, IMO, makes sense and is fine to go with.
Whether that's actually the perfect solution for the FAPI problem or not, should, to my mind, be debated in a follow-up issue, if at all (as I said, to me it makes sense...). As it's perfectly functional and not all un-usable that should not hold up this crazy-awesome-save-the-world-patch.

catch’s picture

Status: Needs review » Needs work

alert() seems reasonable given the alternatives - agree it's sufficient not to hold the patch up on it.

A few minor issues:

It is not possible to drop this block to the given region.

Sounds awkward and drop / to isn't right.

I came up with this, which is still a bit awkward, but might be enough.

The block may not be placed in this region.

// Cached elsewhere.
No it's not :p. I think we should just say 'Do not cache'.

There's also some debug left in the patch.

tstoeckler’s picture

What about:

This block cannot be disabled.

After all, that's what is being tried to do.

Anonymous’s picture

The block may not be placed in this region.

The block may not be moved from this region.

State it so that the attempts to move it will cease.

Gábor Hojtsy’s picture

@tstoeckler: it is not how the JS is implemented. This JS could serve other contrib modules, which would want to limit positioning of their blocks too. I can imagine that for site-specific modules.

@earnie: the error is not that you cannot move the block *out* of one region, but that you cannot move it *into* another. The way it is implemented, it says certain regions cannot host the block. We can make it so that if the dropdown only has one option, we print the message you suggest, but the case in core is not that the dropdown would only have one option.

tstoeckler’s picture

@Gabor Hojtsy: Agreed.

Then let's go with what catch suggested:

The block may not be placed in this region

I chose "cannot" over "may not" in #62, but let's PLEASE not bikeshed over that. Let's just go with what the next patch says.

catch’s picture

Two options then:

"It is not possible to drop this block into the given region." (Gabor's with the tiny grammatical correction I only just realised was necessary to make it work - s/in/into)

And

"The block cannot be placed in this region."

I'm fine with either.

Dries’s picture

1. I'm fine with either of the suggested strings. I'm also fine with the alert() approach.

2. + // Block should go first so that other modules can alter its output during hook_page_alter(). Could you extend this a bit explaining _why_ this is important. I can't immediately see the relation with the rest of this patch.

3. Do we need system_set_content_block()? With the proposed patch, something asks for the main content of the page to be rendered, and then the block system asks for the cached version of that. It sounds like the block system should call drupal_get_page() when asked to render the main content block. I've not studied how hard that would be, but it sounds like a cleaner and more consistent approach. Thoughts?

Gábor Hojtsy’s picture

FileSize
13.78 KB

@Dries/@catch: modified wording to "The block cannot be placed in this region." for simplicity.

@catch: removed the debug code.

@Dries/2: Since we are putting the main content of the page into a block, no module will have access to hook_page_alter() anything meaningful in the page before block module runs. So Moshe says block module should run first. Added this wording to the patch:

/**
 * Set system.weight to a low value for block module.
 *
 * Block should go first so that other modules can alter its output
 * during hook_page_alter(). Almost everything on the page is a block,
 * so before block module runs, there will not be much to alter.
 */

@Dries/3: well, drupal_get_page() is supposed to return the page structure for starting the page rendering. The content part used to have a special significance, so it was requested to be passed on to formulate that part. Now since we make that less of a special case, we are overriding drupal_get_page() for two things: set the page content block value and return the structure it did return before. All use of drupal_get_page() in Drupal currently assume it sets the main content and returns the page structure. If it is to be used by block module to return the value of the content block (vs. returning the page structure), then we are overloading it in one more way. A cleaner implementation would probably be to rename drupal_get_page() to drupal_set_content() or something and *that* would be used by block module then to request that content value. Then there would be remains of drupal_get_page() being called, so that can be integrated into the rendering phase. We can definitely take some time to clean out the API here, it makes sense.

Dries’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

FileSize
23.34 KB

Looked into getting rid of system_set_content_block().

1. Looked at existing use of drupal_get_page(). Only two were valid. One in drupal_render_page() which was a fallback since almost all of Drupal does not return a page structure but assumes the defaults are good. The other was in system_batch_page(), which actually changed stuff in the page structure. All other uses, such as in node module, blog module, etc. could be eliminated because they merely try to wrap the default page structure, which is done later in drupal_render_page() anyway.

2. Broken drupal_get_page() to its two roles. It did look very odd with the above patch, indeed :)

 function drupal_get_page($content = NULL) {
   // Initialize page array with defaults. @see hook_elements() - 'page' element.
   $page = element_info('page');
   system_set_content_block($content);
   return $page;
 }

Basically this was an overcomplicated element_info('page') call with setting the value at the same time. However, as seen above, these two things are not used at once at all but one place in Drupal. So that one place now uses element_info('page'), and setting the page content is now in the new drupal_set_content() [named system_set_content_block() in previous patches].

3. Because there was a drupal_set_content() before, that ought to die :) Hahh. Or we try to find a new name for the above. But drupal_set_content() was *adding* stuff to a *region*, so I've renamed it to drupal_add_to_region() (inspired by the recent rename of drupal_set_html_head() to drupal_add_html_head() - renaming this to drupal_add_region() would suggest this adds regions, while it really just adds stuff to a region). Also, renamed drupal_get_content() to drupal_get_region() along the same lines.

4. Realized there is one advantage of this patch which was not yet in the CHANGELOG. Added this: "Blocks can now return structured arrays for later rendering just like page callbacks."

Drupal 7 installs and works for me with the attached patch. Now let those reviews come!

Gábor Hojtsy’s picture

Actually, there are three places, where the page structure array is needed, all covered by the above patch:

- drupal_not_found() uses it to hide sidebars, so got element_info('page') and drupal_set_content() treatment
- system_batch_page() uses it to hide sidebars AND blocks, so got element_info('page') and drupal_set_content() treatment
- drupal_render_page() is the system fallback for page structure building; when it does not see an element_info('page')-like structure coming in, it uses element_info('page') and drupal_set_content() too

These three places cover where drupal_get_page() was used in a valid case before. Other places just used it before it would have been used anyway.

Dries’s picture

This looks like it rocks.

If drupal_set_content() becomes drupal_add_to_region(), maybe drupal_get_content() should be drupal_get_from_region()?

Or how about: drupal_add_region_content() and drupal_get_region_content()? Just thinking up loud.

Gábor Hojtsy’s picture

drupal_get_content() was already renamed to drupal_get_region(). We can rename them drupal_add_region_content() and drupal_get_region_content(). Not sure the extra "content" is worth it, but we definitely don't use things like "add_to", so it would be more consistent as you suggest.

moshe weitzman’s picture

Status: Needs review » Needs work

Wow - excellent work Gabor. I was a bit worried that implementing Dries' #3 would be complicated but you have done it so smoothly. We are very close to RTBC.

I prefer drupal_set_page_content() over drupal_set_content(). It will also fatal error any contrib code that uses drupal_set_content() which is good as that function's meaning has changed.

I am seeing a NOTICE during install - Notice: Undefined variable: output in /Users/mw/htd/pr/includes/theme.inc on line 682. The theme hook is theme_status_report. Might only happen under certain conditions. Investigating.

I will submit a follow-up patch that really takes advantage of the new 'blocks accept a renderable array' feature. first blocks to upgrade will be login and search forms.

moshe weitzman’s picture

Actually that notice is unrelated. From my perspective, just a rename as I mentioned above is needed before my RTBC. The page content is not a region, but rather a block. I don't see any benefit in trying to wedge it into the drupal_add_region_content().

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
23.47 KB

Thanks for the reviews. Attached patch renames:

- drupal_add_to_region() => drupal_add_region_content() based on suggestion by @Dries
- drupal_get_region() => drupal_get_region_content() based on suggestion by @Dries
- drupal_set_content() => drupal_set_page_content() based on suggestion by @moshe weitzman

Now these function names look pretty consistent. Uploaded patch tested on Drupal 7 installation and some pages. Looks like working.

Dries’s picture

Status: Needs review » Reviewed & tested by the community

Awesome. Marking this as RTBC so some other people can help review before I proceed committing this.

moshe weitzman’s picture

I have one more enhancement I am going to try to get in here. I can't get to it for a day or two or three, so anyone is welcome to commit before this before I make a patch. I agree that this is RTBC.

I want drupal_render_page() to save a reference to the main page content in $page[#page_content]. then modules will change that element during hook_page_alter(). The contents of that element get moved to the new system-main block just before it gets rendered. Or something like that. It is a little messy, but hopefully no messy than the indirection we have in the current patch.

Dries’s picture

I've committed this to CVS HEAD. Marking this 'code needs work' for two reasons:

1. Let's update the upgrade instructions
2. Waiting for Moshe's follow-up patch.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
moshe weitzman’s picture

I must be missing something about php's references because I can't get changes to a new $page['#page_content'] element to persist. The changes are happenning during hook_page_alter. I give up for now. Lets just add some docs and mark this issue fixed.

xmacinfo’s picture

I've updated through CVS Drupal and run update.php.

Unforfunatly, the content do not display for any page. I can't even display the Blocks admin content!

How do I force the display of the content block? Do I need to delete the database and reinstall?

catch’s picture

Title: Make the main page content a real block » Make the main page content a real block (update.php broken)
Category: feature » bug
Priority: Normal » Critical

Sounds like update.php is broken. Probably need to reinstall for now. Changing status.

webchick’s picture

Well. update.php works fine. The problem is your site doesn't once you've run update.php. ;)

Uh oh, spaghettio

webchick’s picture

Title: Make the main page content a real block (update.php broken) » Make the main page content a real block (update path broken)
webchick’s picture

Oh. Weird. Why was the upgrade path for this added to an older function instead of a separate one made? That's probably what the problem is.

webchick’s picture

Title: Make the main page content a real block (update path broken) » Make the main page content a real block
Issue tags: +Needs documentation

Yeah, confirmed that's it.

@xmacinfo: Reinstalling from scratch is easiest.. you can also remove the 'help' row in your block table, remove the two queue tables, and set system.schema to 7020 to simulate the update. I did that and it does work ok.

Removing the update stuff from the title. Still needs docs, per #79.

xmacinfo’s picture

Please note that after updating though CVS my Drupal files and then doing the update.php like I did in #82, the update page give me this message:

No pending updates.

I then visited the modules page hoping the cache would rebuild and solve the issue, to no avail.

Do this patch need an update?

Yes, for now I'll delete the database and reinstall. :-)

tstoeckler’s picture

Gabor from #40:

Given that we will keep modifying the same update function to achieve an efficient upgrade path and to avoid copy-pasting the same code multitude times...

It seems all the "Footer message as block", "System help as block", "Mission statement as block" and finally "Content as block" had large overlaps in their update function so they were all added to one big "Make everything into a block" update function. That's why this patch didn't introduce a new update function and just added itself to an existing one. And that's what broke your site, xmacinfo.

Gábor Hojtsy’s picture

I've consulted catch and others in similar issues, and got the advice that I should not care about D7-D7 updates just yet. There would have been lots of code copied in update functions, if we'd add this in another one. More queries, slower code, more theme cache rebuilds, you get it.

So the needs work is only on needing documentation, so I'll look into that later today.

catch’s picture

Gabor's right, I think we just all forgot when we ran cvs update.

Gábor Hojtsy’s picture

Added this to the developers update doc at http://drupal.org/node/224333#drupal_set_content and http://drupal.org/node/224333#drupal_get_page I hope it covers all the use cases properly. Going to document in the theme upgrade guide too shortly.

<h3 id="drupal_set_content">Renamed drupal_set/get_content(), removed drupal_get_page() and added drupal_set_page_content()</h3>

<a href="http://drupal.org/node/428744">(issue)</a>. In Drupal 6, you could add content to a region with drupal_set_content() and request the full contents of a region with drupal_get_content(). These were renamed in Drupal 7 to drupal_add_region_content() and drupal_get_region_content(). Apart from renaming them, functionality is the same.

Drupal 6:
<?php
// Add our own text to the footer.
drupal_set_content('footer', 'Adding custom text to footer');
// Get the complete footer contents.
$full_footer = drupal_get_content();
?>

Drupal 7:
<?php
// Add our own text to the footer.
drupal_add_region_content('footer', 'Adding custom text to footer');
// Get the complete footer contents.
$full_footer = drupal_get_region_content();
?>

<h3 id="drupal_get_page">Removed drupal_get_page() and added drupal_set_page_content()</h3>

<a href="http://drupal.org/node/428744">(issue)</a>. In Drupal 6, drupal_get_page() had a dual purpose. It was used to set the main page content text of the page and get an array of page attributes which could be modified (such as to hide the sidebars). 

Drupal 7 made the page content an actual block, so setting the page content is done with the new drupal_set_page_content(). This can now take a string (rendered page HTML) or a renderable array. By default, Drupal runs the output of page callbacks through rendering, so returning a string or array from your page callback is preferred. Drupal will call drupal_set_page_content() later anyway.

The only case when you need to call drupal_set_page_content() is when you need to alter page options. In that case, use element_info('page') to request the default page attributes, and modify them as needed. Return with that array from your page callback and set the page content with drupal_set_page_content().

To reiterate, the basic use case did not change:

Drupal 6:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return $output;
}
?>

Drupal 7:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return $output;
}
?>

If you've used drupal_get_page() as a wrapper on your return value, but did not alter its output, just remove it:

Drupal 6:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return drupal_get_page($output);
}
?>

Drupal 7:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return $output;
}
?>

If you need to alter page attributes, this is how you do it:

Drupal 6:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal, hide blocks.
  $page = drupal_get_page($output);
  $page['#show_blocks'] = FALSE;
  return $page;
}
?>

Drupal 7:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Set page content for Drupal.
  drupal_set_page_content($output);
  // Return page attributes, hide blocks.
  $page = element_info('page');
  $page['#show_blocks'] = FALSE;
  return $page;
}
?>

In short, always return the main page content in a string or renderable array unless you need to modify page attributes. In this later case, use drupal_set_page_content() and element_info('page') and return the modified attributes.
yched’s picture

"In Drupal 6, drupal_get_page() had a dual purpose..."
Er, is it me, or I can't find any drupal_get_page() in D6 ? Wasn't this a D7 addition ?

Gábor Hojtsy’s picture

Erm, right. Editing doc. :) Looked at D6 system_batch_page() and it uses theme('page', $output, FALSE, FALSE); Looks like there is no existing update doc on that being removed in favor of drupal_get_page(), so I'll just go ahead and document the theme('page') depreciation.

Gábor Hojtsy’s picture

Thanks again for the doc review yched. Updated text (with updated anchor at http://drupal.org/node/224333#theme_page). I've titled it in an unconventional way since you will probably not want to use drupal_set_page_content() is almost any case.

<h3 id="theme_page">Instead of theme('page', ...), think of drupal_set_page_content()</h3>

<a href="http://drupal.org/node/428744">(issue)</a>. In Drupal 6, theme('page', ...) had a dual purpose. It was used to set the main page content text of the page and set some page display options (such as to hide the sidebars). 

Drupal 7 made the page content an actual block, so setting the page content is done with the new drupal_set_page_content(). This can now take a string (rendered HTML) or a renderable array. By default, Drupal runs the output of page callbacks through rendering, so returning a string or array from your page callback is preferred. Drupal will call drupal_set_page_content() later anyway.

The only case when you need to call drupal_set_page_content() is when you need to alter page options. In that case, use element_info('page') to request the default page attributes (an array in Drupal 7), and modify them as needed. Set the page content with drupal_set_page_content() and return with that array from your page callback.

To reiterate, the basic use case did not change:

Drupal 6:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return $output;
}
?>

Drupal 7:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return $output;
}
?>

If you've used theme('page', ...) as a wrapper on your return value, but only passed on the output (not any of the other parameters), just remove it. This was already the suggested behavior in previous Drupal versions, so it is time to get rid of theme('page').

Drupal 6:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return theme('page', $output);
}
?>

Drupal 7:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal.
  return $output;
}
?>

If you need to alter page attributes, this is how you do it:

Drupal 6:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal, hide blocks.
  return theme('page', $output, FALSE);
}
?>

Drupal 7:
<?php
function mymodule_page_callback() {
  // Assemble page output here...
  
  // Set page content for Drupal.
  drupal_set_page_content($output);
  // Return page attributes, hide blocks.
  // Similarly, use #show_messages for the migration
  // of the third argument of Drupal 6's theme('page', ...).
  $page = element_info('page');
  $page['#show_blocks'] = FALSE;
  return $page;
}
?>

In short, always return the main page content in a string or renderable array unless you need to modify page attributes. In this later case, use drupal_set_page_content() and element_info('page') and return the modified attributes.
Gábor Hojtsy’s picture

Status: Needs work » Fixed

Also documented related theme changes at http://drupal.org/node/254940#content-region:

<h3 id="#content-region">Content region is now mandatory, main page content became a block</h3>

<a href="http://drupal.org/node/428744">(issue)</a> In Drupal 6 and before the $content variable in page.tpl.php contained the main page content appended with the blocks blocks positioned into the content region (if you had that region defined).

In Drupal 7, $content became a full region and is now mandatory in all themes. This new requirement was set up so that when enabling new themes, Drupal knows where to put the main page content by default.

In Drupal 6, it was only possible to put blocks after the main page content in this region. The only way to put blocks before the main page content was to define a specific region for that purpose. Drupal 7 now makes the main page content its own block. This makes it possible to put blocks before or after the main page content in the region without hacking in a new region.

If you relied on the fact that blocks were only put in the sidebars and therefore got their styling via just a .block class selector or something similar, now you need to be aware that because the main page content is now wrapped by markup from block.tpl.php, you might need to revisit your theme CSS files, and limit your blocks styling to certain regions by making the selectors more specific.
Gábor Hojtsy’s picture

Actually, fixed some language, repetition, etc. Feel free to edit to become better :)

<h3 id="content-region">Content region is now mandatory, main page content became a block</h3>

<a href="http://drupal.org/node/428744">(issue)</a> In Drupal 6 and before the $content variable in page.tpl.php contained the main page content appended with the blocks positioned into the content region (if you had that region defined).

In Drupal 7, $content became a full region and is now mandatory in all themes. This new requirement was set up so that when enabling new themes, Drupal knows where to put the main page content by default.

In Drupal 6, it was only possible to put blocks after the main page content in this region. The only way to put blocks before the main page content was to define a specific region for that purpose. Drupal 7 now makes the main page content its own block. This makes it possible to put blocks before or after the main page content in the region without hacking in a new region.

If you relied on the fact that blocks were only put in the sidebars and therefore got their styling via just a <*code>.block</*code> class selector or something similar, now you might need to revisit your CSS files. Because the main page content is wrapped by markup from block.tpl.php, the <*code>.block</*code> selector will match that too, so you need to limit your blocks styling to certain regions by making the selectors more specific, such as <*code>#left-sidebar .block</*code>.
Gábor Hojtsy’s picture

Turns out that the element_info('page') based attribute setting is not too elegant for contrib modules. Also, core modules can be simplified, page callbacks straightened if we get back to only ever returning a content string or array, but not a page structure array. In upgrading a contrib module, it turned out that the state after this patch is still quite inelegant. Look at #470726: DX: Clean up page callback return values for my follow up and deeper explanation.

yoroy’s picture

Folow up: we need to switch this block on for all themes in core: #473080: Blocks are not inherited when switching themes

webchick’s picture

FYI: This patch broke Vertical Tabs's styling: #473878: Vertical tabs are totally hosed

Any ideas?

xmacinfo’s picture

#110 was fixed today in #473878: Vertical tabs are totally hosed! Awesome!

Status: Fixed » Closed (fixed)

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

kenorb’s picture

Gábor Hojtsy:
If you use this code in Drupal 6:

function mymodule_page_callback() {
  // Assemble page output here...
  
  // Return page output for Drupal, hide blocks.
  return theme('page', $output, FALSE);
}

If not theme page twice? If page_callback returns already rendered data into index.php to theme it again?
Question from: #939040: Panels returns NULL from menu_execute_active_handler()