Add fallback for main content block rendering

It's worse than we thought. If someone manages to accidentally disable the block.module during the admin/build/block page, the resulting screen after submit looks like this:

Whoever thought that both allowing the Drupal block module to be disabled and creating a block for page $content was a GOOD IDEA, EH? #fail.

The idea is supposedly to be able to completely replace the block module with something else... I don't think it makes a lot of sense either.

Ok, DamZ, so let's make it optional for other modules to shut it off, but non-optional for site administrators to disable via the admin/build/block page. After all, if another module wants to take over the display of all block output sitewide, can't they just change the block.module to status = 0 in the DB?

I've thought about this for the last hour, and it seems to me that the immediate need is to make the block module required again so it can't accidentally be shut off via admin/build/block, and then devise a method for contrib modules like Panels to move the block module out of their way if they so desire.

I can't think of a single UI scenario where I could shut off the block module using admin/build/block and turn on another module like Panels via admin/build/block and know that I was replacing one with the other. Thus, admin/build/block is not the place where the block module should be disabled. I don't think it should even appear here, but instead the option to disable it should appear "somewhere else".

What are the drawbacks to setting 'required' on block module again?

The idea was that since nothing in Drupal really depends upon the block module (originally anyway) that it should be optional to ease performance on simple sites. Then we introduce a dependency on block module for every page, which was, perhaps a great idea as far as usability, but terrible as far as flexibility goes.

In theory I suppose that it wouldn't be that hard for some contrib module to do some hook_page_alter and theming stuff with preprocessors to get the content, footer, etc back on the page without requiring the block module, and it could be up to that module to disable block module.

I don't like the idea of Panels (as an example of a module that would allow block to be disabled) messing with my system table. I would rather like 1. an additional layer of warning, e.g. when you try to disable block, you get a big fat screen in your face, saying: "If you click "Proceed", your computer will explode!" and 2. a page on d.o that comes up top when I search for "block module disabled" that says: "Go to phpmyadmin, do this, do that, be happy!"

@tstoeckler in #6: I'm adamant that no amount of warning or in-your-face confirmation is allowable before shutting off block module without a replacement system already in place and switched on. To do so would be tatamount to allowing a fledgling glider pilot into the seat of a 777 with nothing more than a warning that "These 12 switches make the plane stop flying if you flick them in the wrong order, but you have to toggle them once in a while or the plane will also stop flying. Have fun up there!"

You are adamant about not letting a contrib module near your system table. However, just let me point out that a module's install hook can and does change things in all kinds of tables. I'm just sayin! :) Also, not everyone has PHPmyadmin, and some of them cannot install it by themselves or comprehend it even if they could. I have spent many hours in IRC helping such people recover from dastardly Drupal Things.

I'm re-introducing the block module as required with this one line patch, but not reverting anything else that's been done to support the block module as optional. In this manner, we can continue to forge ahead without the possibility of accidentally breaking our sites, but this one line is easily removable when the time comes to begin discussing a UI for allowing other modules to step in and substitute themselves for block.module.

You can't fool for AutoBot! Here's the patch again, but this time with a test. (Gosh, webchick has that thing *trained*!)

This happens not only when the block module is disabled, but when the main content block visibility options are altered such way that cause the block does not appear in admin. For example, somene may change the Content type visibility settings, by mistake, and there is no way to undone that action.

Block module optional is a pretty good feature from developers point of view, would be good to find out other solution that keep block.module not required.

I wonder if required = TRUE can be hook_system_info_alter()ed out by other contributed modules. Worth a test.

Er. And by that I meant "Someone should make a contributed module that attempts to do that and report back their results" not "We need a SimpleTest for it." :)

I don't completely follow the objections here. There have always been ways to screw up your site and we are starting to add drupal training wheels all over our app. At some point, that has to stop.

I expect that we will see install profiles that ship with alternate block implementations, and sites that have no need for a block UI and just populate the regions themselves in code. It is a pretty sweet approach for a custom site, to be honest. hook_page_alter() is way more dynamic than block module which is still stuck in drupal3 land. I don't think we should get in the way of these sorts of sites.

Tagging.

What about a set message, after you disabled the block module.

"Dude, did you really want to disable the block module? Enable it again"

I'm wondering if something like the following would work?

/**
 * Implement hook_page_alter().
 */
function system_page_alter(&$page) {
  // If no module has populated the "content" region of the page, provide a
  // fallback implementation which inserts the main page content into this
  // region. This allows the site to still be usable even if no modules that
  // control page regions (for example, the Block module) are enabled.
  if (empty($page['content'])) {
    $page['content'] = drupal_set_page_content();
  }
}

This would need some further thought as to how it would interact with block visibility settings and the like, but I think the basic idea works.

@webchick's idea in #11 sounds promising too, but I'm not sure how that would work with installation profiles -- somehow the contrib module's hook_system_info_alter() implementation would have to get called before Drupal starts installing the required core modules, I think, which sounds tricky...

Oh, here are screenshots showing what happens when you disable the block module with the code from #16 applied. I think this matches what users would expect to happen, and it's easy to recover from if they disabled the module by mistake.

If it seems like we want to go this route, I can obviously turn the above code into a patch.

Yes, I like that very much. Block visibility settings would have to be avoided, though, as I can just set my "main content" block to not appear on admin/build/block and then the same thing happens as before. Now that might be an utterly stupid setting, but people disabling block module off the top of their head without any reason to, ...well

@David's approach looks good to me. It does't have to be 100% bullet proof. Very few users will ever get into this situation in the first place.

#16 looks fine to me too, much better than a warning.

+1 to #16's approach. David, go ahead and roll a patch for it, since it was your idea, and I'll test. Oh, and Moshe?

It doesn't have to be 100% bullet proof. Very few users will ever get into this situation in the first place.

I'm free to point all users in the forums + IRC who have this problem straight to your doorstep now, huh? ;)

OK, here it is as a patch, but it looks like there are a couple issues:

1. When block.module is disabled, bulleted lists are aligned wrong in the content region (see screenshot). I guess this is an existing issue where there is CSS in Drupal that relies on the main page content being inside a block? I didn't really have time to track it down.

2. I don't think I explained very well the issue with block visibility settings. What I actually meant above was that it makes them not work sometimes (for the main page content block). If you were to set the block visibility so that it did not show on some pages or for some users, and if it were the only block in the content region, then system.module will notice that the region is empty and therefore put the same content in that region, even though you asked for it not to be there. It sounds like an edge case to me (and there are workarounds for it, via either the blocks user interface or code), but maybe there are cases where it is needed - for example, I looked through #428744: Make the main page content a real block and it appears that comments #44-#46 were discussing a use case for this.

3. Similarly, if you drag the main page content block into another region of the page and in doing so leave the "content" region empty, system.module will fill it, so in that case you will have two copies of the main page content showing on the screen! (Also sounds like an edge case to me, but worth thinking about.)

I think we're only trying to prevent a nuclear holocaust in the administrative section(s), so why not perform this $page #content block replacement only on 'admin/' pages of the site?

That way, if the site's $page content is somewhere else, or is even switched off using a 'this block only shows up on...' setting, at least the admin pages of the site can print their stuff to the page.

I prefer the patch in #22, as it allows the site to fully function when all non-required modules are disabled.

Sites that only use a single block, won't need the block module anymore, yet basic content output is still guaranteed.

BTW: is block.module a requirement for menu.module?

Here is my take on the problem:
This patch will only show the main content in "content" if all visible regions are empty.
Otherwise the module filling the regions is responsible for showing the content.

(to awnser my own question: no, it shoudn't be.)

Patch review of #27
Nice try, good conceptual execution, and simplistic, but what if a page *doesn't* want any blocks? What if the entire site doesn't want any blocks? What if I want to turn of the block.module entirely and not have anything controlling my blocks?

I think the intent here is to make sure the the admin/ pages, and specifically the admin/build/block page can have it's $content displayed. Why not take your idea from patch #27 and apply it only to paths that begin with admin/ ?

As to code review, you don't really *need* to set a variable for $region_filled, since the statement if(!empty($page[$region])) already evaluates as a boolean.

<?php
+    if(!empty($page[$region])) {
+      $region_filled = true;
+    }
+  }
+  if (!isset($region_filled)) {
+    $page['content'] = drupal_set_page_content();
?>

Could become

<?php
+    if (empty($page[$region])) {
+      $page['content'] = drupal_set_page_content();
+    }
?>

[Also note that you need a space between the 'if' and the '('.]

Executive Summary
Needs a minor rewrite, but it's very close to a workable solution.

@Senpai, there is nothing about the patch that assumes the block module is enabled, right?

I think #27 works fine and is a neat solution. The CSS issue I mentioned in #22 is still there, so we should probably fix that, but this seems like the right approach to me.

To clarify, the code added by the patch only "triggers" when there is nothing in any of the site's non-hidden page regions... The reason I think it's OK is that I'm having trouble imagining a page in Drupal that would intentionally put things in the hidden regions only (which for a typical theme basically just includes the region where the toolbar goes).

So I think it's OK, unless there's a good use case for that that I'm not thinking of?

Senpai, this kōhai notices your logic is faulty. ;)
We are working with a foreach here. if the second element is filled, and the third element isn't, your code triggers Senpai, yet the code from this kōhai doesn't.

---

in my opinion, the CSS issue is something that can be solved in the themes themself (thus being another issue (for each theme)) or someone should come up with a more "drupalish" way.

head broke -- resetting status

Coming back to my comment #29 again, I had a talk with kscheirer right after I posted that, and I want to retract my statement. I realized immediately after posting that it was in fact inside a foreach loop and thus my comment was bunk. But, there's another problem which makes this not so good of an approach.

Why would we desire to loop through upwards of 50 theme regions in order to see if any of them are populated, on every page load, for every site in the world, before delivering the goods? No, this is not so good. It would be improved by ditching the foreach() and using a while() so that it would bail out if it found one, but even that is not the best solution.

Really, why can't we go with Rothstein's patch that only substitutes the admin page's $content if nothing else is found to be in control? That's what we're really trying to fix here, true?

It's being assumed that the theme used will have a region named 'content'. Is that a requirement for themes?

Wait, David Rothstein endorses patch #27? Ok, David, I like the approach, but I really don't think that we need to be checking for all of this stuff. The purpose of this issue, as the title suggests is to prevent someone from irrevocably breaking their site by accidentally switching off the block module.

This bug only manifests itself on the admin/build/modules page, and in that very limited scope, only the $content of that one page needs to be presented to users should the block module be switched off. That's why I'm saying that your original idea is really good, cause in any other case, it's not this issue's problem.

So, gang, do with it as you will, but remember that the problem you're trying to fix is very small.

[Yes, dropcube, $content is a required region for all themes.]

This patch implements other approach:

If a module that control page regions, such as block.module inserted the main content block in at least one region, then the module must indicate that, for example, by setting $page['#system_main_block'] = TRUE.

The module in question, should implement a mechanism to detect if the main content block was already inserted, and let the other modules know about. For block.module, that won't be hard, since it knows well the structure being generated:

    $system_main_block = FALSE;
    ...
    if ($blocks = block_get_blocks_by_region($region)) {
      $page[$region] = $blocks;
      $system_main_block |= isset($blocks['system_main']);
      ...
      ...
     // Let other modules know if the main content block was already inserted.
    $page['#system_main_block'] = $system_main_block;  
    }

Then, system.module provides a fallback implementation which inserts the main page content into the 'content' region if no other module inserted it. This allows the site to still be usable even if no modules that control page regions inserted the content.

If the block module is disabled, and your module is who populates the regions, then you should set $page['#system_main_block'] = TRUE;, even if you didn't insert the main content block actually. This would be a way to say, hey system.module, don't worry, I know what I am doing, don't provide your fallback, thanks!.

block.module will say the above only if it's enabled, and if it detected that the main content was set.

Tagging. Whatever the approach used, probably it introduces an API change.

Updated patch that implement the approach proposed in #38.

- Tests added, with multiple situations.
- Changed the control property, now it's #no_render_main_content ( $page['#no_render_main_content'])

Is there any other situation that can be added to the tests ? Please, suggest.

@Pasqualle: If we decide to use an approach like the proposed in #38, it's something that core/contrib modules implementing this feature should know and use.

I'm still not 100% convinced about the property name... suggestions ?

- Changed control property to #has_main_content, seems to me more clear and understandable.
- In block_page_alter(), the control property is not overwritten, but merged with the current value, so that it doesn't interfere with other modules that might have run before.
- Added the property to system_elements(), default to FALSE:

$type['page'] = array(
    '#show_messages' => TRUE,
    '#has_main_content' => FALSE,
    '#theme' => 'page',
);

Note that this approach is also compatible with: #558958: Add a hook_page_build() that runs before hook_page_alter().

I think this is getting too complex for solving the problem at hand.

I like senpai's point in comment #29: just display the fallback content on the block admin page and the module admin page, to allow people to recover from a bad setting.

I think this is getting too complex for solving the problem at hand.

I like senpai's point in comment #29: just display the fallback content on the block admin page and the module admin page, to allow people to recover from a bad setting.

Then, the fallback should also be implemented in better_blocks.module admin page, panel.module admin page, better_still_blocks.module admin pages, ...., etc... to recover from a bad setting when any of these modules is the module being used to populate page regions. To implement this, we should deal with paths, arguments, etc ... and may become ugly very easy.

I think, the block.module issue is a particular case of a major issue: If any of the modules added the 'main content' to the page, then the system should provide a fallback to add the 'main content'.

In the other hand, if a module ensure it's adding the content, then it should set $page['#has_main_content'] = TRUE; isn't simple?

I am marking the issue back to CNR, we have several patch here that needs review!

Yeah, I agree - solving this bug for only some pages in Drupal but not others doesn't really seem like a complete solution.

The patch in #42 looks like it should work, although it's a little unfortunate that each page management module has to remember to deal with this setting.

I'm still not sure what's wrong with the patch in #27, though? It seems to me like it took my original patch and neatly fixed all the problems with it :) The foreach() loop through the array doesn't seem like it would be particularly expensive, and it can be made a bit less so by adding a break statement in there. As above, the only concern I can think of is that maybe we don't exactly know how contrib modules are going to use a theme's "hidden" regions, and in the event that a page is ever intentionally trying to only put content in those regions, the approach would fail.

I actually like #42. It's not at all complicated, the patch mostly contains patches, the actual code added is, like, 10 LOCs. It's flexible yet simple. +1 for #42.

But why not just do it for all pages if block.module is disabled. It's the same amount of code and doesn't do any harm. It actually makes your site quite functional, you just have no blocks.

@Pasqualle

The main problem is, that any of these patches does not make your site fully functional if the block module is disabled.
We only need to fix the admin pages, that they are usable enough to re-enable the block module (or change any module settings)..

You site is actually functional, of course, without blocks. But, because you want it without blocks, just because your module is going to insert the content in the regions, or because you really disabled the block module, not but mistake. The main reason to have block.module optional is to be able to disable it, if you need.

because we do not really want to mess with page rendering, there could be a valid use case when the content region is empty.

Yes, there could be, of course. Suppose that your module, which manages the page regions, want to leave the content region empty, then you should do$page['#has_main_content'] = TRUE;. You can do this in a menu callback function, in hook_page_build(), hook_page_alter(), etc... system.module will respect that, and will leave the content region empty, even if there is no content actually.

----
In general, we need a flexible yet simple solution, that ideally does NOT deal with paths and arguments.

Note, the patch in #42 contains a bunch of tests with several use case. Is there any use case that is not covered? Suggestions welcome.

@Pasqualle

I do not see why would we create another #has_main_content variable, when 'content' is a required region and all admin pages use this region..

Not necessarily, the main content is exported as a block, and can be put in any region, even in admin pages. The introduction of another control property #has_main_content, is part of the solution proposed.

At #47, you said:

The main problem is, that any of these patches does not make your site fully functional if the block module is disabled.

At #51:

Drupal does not need to be fully functional without Block module, there is no such requirement..

I find this contradictory.

just display the fallback content on the block admin page and the module admin page, to allow people to recover from a bad setting.

how about, we ensure it is not a bad setting ;) also, how do we get there? (besides typing the url.)

The main problem is, that any of these patches does not make your site fully functional if the block module is disabled.

Actually, some of these patches does make your site fully functional, just without the block module.
For example menu.module still manages the primary/secondary menu.

Drupal does not need to be fully functional without Block module, there is no such requirement.

However, non-block functionality should still be fully functional, and that is a requirement (or it should require the block module.)
For example, the node or user module should still be fully functional when the block module has been disabled! (just without blocks.)

there is also a problem with css as described in #22.

the CSS issue is seperate issue, as themes still assume the blocks module is always availble.

added an optimized version of the patch in #27 (still needs tests.)

added tests.

fixes the failure (my stupid mistake), I'll create a new issue for the exceptions. (#564084: Warning: Invalid argument supplied for foreach() in system_region_list())

With the following case the patches in #53, #54, #56 will fail:

block.module is enabled.

region[content_top] <= main block content (assigned to this region, but the block visibility settings were configured mistaken, and the block is NOT displayed in admin pages)
region[content] <= custom block + views block ($page['content'] is NOT empty, so no fallback is provided by system module)
region[footer] <= custom block with footer text

Also, seems that you removed from the tests the part where the main content region is enabled in other region different than content.
-----------

We can not assume anything, let the modules that manage page regions decide if the system fallback should be triggered. The modules should know well what to do to decide. In case of block.module, it's easy, it knows well if the 'main content block' was added to the page structure or not. In the other hand, better_blocks.module may be using a totally different approach, it will lknow well what it's doing, but system.module won't.

You are trying to do something what is not possible and was never required..

That's not true. It is possible. As of now, Block module is optional. That means people can uncheck one checkbox on admin/build/modules and kill their entire site. With no way back, even, if they don't have DB-access. That's the situation now.
This patch fixes that "bug" and completes the "optional-ness" of Block module in that you can then run your site without Block module (which in common English would be defined as "optional").

If I disable the block module that means, that I do not want to print any block in any region.

That's true. But there might be a good reason I want to do that. Maybe Panels in Drupal 7 will completely bypass Block module or the Dashboard module or the Better_blocks module or whatever. Disabling block module only means, I don't want that specific implementation of handling my page display. If we don't want people to disable block module, we should make it required, not (more or less) force them to reenable after disabling.

If I disable the block module that means, that I do not want to print any block in any region.

The patch #47 is absolutely enough to fix the issue, which is "create an option in admin interface to re-enable the disabled block module".

The patch in #47 is very restrictive. It's forcing to have at least a block in the 'content' region, even with the block.module enable. I may have the 'content' region empty, and the 'main content' block displayed in a region named 'main content' or 'content top', etc... Also check the case in #57, completely will fail.

Disabling the block.module doesn't mean I don't want to print any block, it just mean I DO NOT want block.module to populate regions in my page.

For that reason, we need a fallback, to still be able to see the 'main content' block, when no module is populating regions. The issue here goes beyond the block module, and beyond /admin specific paths.

The main issue here is to decide when the fallback provided by system module should be triggered

Do we agree on that ?

here, a much simpler patch, also rerolled against HEAD.

now, it only triggers if nobody has called drupal_set_page_content() yet to retrieve the page content, and as an added feature, a module can reset the "fuse"

+++ includes/common.inc	31 Aug 2009 20:23:53 -0000
@@ -3935,10 +3935,12 @@ function drupal_alter($type, &$data) {
+  $system_main_page_content_display = &drupal_static('system_main_page_content_display', false);
...
+    $system_main_page_content_display = true;

(minor) Constants should always be all-uppercase, including pre-defined PHP constants like TRUE and FALSE.

- There were other tests in the patch, can you include them in your patch ?
-----------------

The downside I see with this approach, it that we can NOT guarantee that the 'main content' block get actually rendered. A module could call drupal_set_page_content(), get the content, and decide not rendering it. Or, a module implementing hook_page_alter() could remove the block from the $page structure. So, you will need to do the following to reset the control variable:

  $system_main_page_content_display = &drupal_static('system_main_page_content_display', FALSE);
  $system_main_page_content_display = TRUE; 

It does not seems simpler to me, it's similar to the approach in #42, where you have to set $page['#has_main_content'] = TRUE; ?> if you don't want the fallback triggered.

+    // Create and login admin user
+    $this->admin_user = $this->drupalCreateUser(array(
+      'access administration pages',
+      'administer site configuration',
+      'administer blocks',
+      'administer nodes'
+    ));

Missing a trailing comma on the last array item.

- the other tests were NOT part of the fallback implementation, but should be part of the block module test, if not already availble (test duplication?)

resetting is as simple as

<?php
  drupal_static_reset('system_main_page_content_display');
?>

the simplicity of this patch is in the fact that you only have to special-case situations where you're getting the content, yet not rendering it. (normal case (99%) no need to set something special.)

updated patch with other comments.

I haven't tested, but I think this approach looks the sanest of all the ones so far, in terms of simplicity whilst being new-user-proof and still allowing for funky new page layout modules. The code, tests and comments look good to me too.

Ok, looks better now.

- system_main_page_content_display seems a little lengthy, can we use a short name for that variable.
- Consider adding the following tests:

+    // Test that content assigned to regions is entirely controlled by a
+    // module. The system_test module will generate content in a region of the
+    // page, and no other region will have content assigned.
+    $this->drupalGet('system-test/page-alter');
+    $this->assertRaw('id="system-test-page-alter"', t('Content rendered by a module.'));
+
+    // Enable block.module.
+    $edit = array();
+    $edit['modules[Core][block][enable]'] = 'block';
+    $this->drupalPost('admin/config/modules', $edit, t('Save configuration'));
+    $this->assertText(t('The configuration options have been saved.'), t('Modules status has been updated.'));
+    module_list(TRUE);
+    $this->assertTrue(module_exists('block'), t('block.module enabled.'));
+
+    // Enable the main content block, to be displayed in the footer region.
+    $edit = array();
+    $edit['system_main[region]'] = 'footer';
+    $this->drupalPost('admin/structure/block', $edit, t('Save blocks'));
+    $this->assertText(t('The block settings have been updated.'), t('Block successfully move to footer region.'));
+
+    $this->drupalGet('admin/content');
+    $this->assertField('operation', t('Main content block found.'));
+    $this->assertRaw('id="block-system-main"', t('Main content block rendered by block.module.'));
+
+    // Set the block to be hidden on any user path.
+    $edit = array();
+    $edit['pages'] = 'user*';
+    $this->drupalPost('admin/structure/block/configure/system/main', $edit, t('Save block'));
+
+    // Request a user* page
+    $this->drupalLogin($this->web_user);
+    $this->drupalGet('user/' . $this->web_user->uid . '/edit');
+    $this->assertField('mail', t('Main content block found.'));
+    $this->assertNoRaw('id="block-system-main"', t('Main content block rendered by system fallback.'));
+
+    // Request other page different to user*.
+    $this->drupalGet('node');
+    $this->assertRaw('id="block-system-main"', t('Main content block rendered by block.module.'));

The above will check:
- The fallback, when other module different to block is population the regions (system_test.module in this case), but the 'main content' should not be rendered, incidentally the module doesn't want it generated.
- The fallback won't be triggered if the main block if in other region, for example footer.
- The main content will be rendered by the fallback when the block.module is enabled, but the block is not visible.

After this, I will be comfortable marking this as RTBC.

rerolled agains HEAD, added some more tests (without using block.module - system should work without it.)

Looks good.

One minor point, can you rename the statically cached variable from system_main_page_content_display to something like system_main_content_added ?

here you go.

i've accidentally disabled the block.module.
please commit this patch.

+++ includes/common.inc	2 Sep 2009 17:15:34 -0000
@@ -3935,10 +3935,12 @@ function drupal_alter($type, &$data) {
 function drupal_set_page_content($content = NULL) {
   $content_block = &drupal_static(__FUNCTION__, NULL);
+  $main_content_display = &drupal_static('system_main_content_added', FALSE);
   if (!empty($content)) {
     $content_block = (is_array($content) ? $content : array('main' => array('#markup' => $content)));
   }
   else {
+    $main_content_display = TRUE;
     return $content_block;
   }
 }
+++ modules/system/system.module	2 Sep 2009 17:15:49 -0000
@@ -3111,6 +3111,20 @@ function system_page_build(&$page) {
+  $main_content_display = &drupal_static('system_main_content_added', FALSE);
+  if(!$main_content_display) {
+    $page['content']['system_main'] = drupal_set_page_content();

I don't get what's happening here.

We need to add some inline comments explaining why we set $main_content_display to TRUE when the page content is retrieved instead of set. Potentially more.

+++ modules/system/system.module	2 Sep 2009 17:15:49 -0000
@@ -3111,6 +3111,20 @@ function system_page_build(&$page) {
+function system_page_alter(&$page) {

Who ensures that system_page_alter() is executed *after* any other potential page/block management module solution in contrib?

Panels will most likely want to use hook_page_alter() in D7, but system_page_alter() runs first.

+++ modules/system/system.module	2 Sep 2009 17:15:49 -0000
@@ -3111,6 +3111,20 @@ function system_page_build(&$page) {
+  if(!$main_content_display) {

Missing space after if.

This review is powered by Dreditor.

Marking CNW for sun's review.

Shouldn't panels use hook_page_build()?

@sun:

Who ensures that system_page_alter() is executed *after* any other potential page/block management module solution in contrib?

Panels will most likely want to use hook_page_alter() in D7, but system_page_alter() runs first.

The modules themselves must ensure that their hooks run before sysmtem.module hooks. For example, block.module does this when installed.

function block_install() {
  drupal_install_schema('block');

  // 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.
  db_update('system')
    ->fields(array('weight' => -5))
    ->condition('name', 'block')
    ->execute();
}

Why don't we ensure it simply runs last in the process? http://api.drupal.org/api/function/drupal_render_page/7 already contains code at the beginning that makes sure a menu page callback returning a string is properly output. It would make sense to put this final validation after hook_build_page() and hook_page_alter() have been invoked.

adapted for dropcube and sun' review.

added missing space from sun' review... sorry sun ;)

and now the right patch... lol!

+++ includes/common.inc	9 Sep 2009 21:26:47 -0000
@@ -4013,10 +4013,17 @@ function drupal_alter($type, &$data) {
 function drupal_set_page_content($content = NULL) {
   $content_block = &drupal_static(__FUNCTION__, NULL);
+  $main_content_display = &drupal_static('system_main_content_added', FALSE);
@@ -4034,6 +4041,8 @@ function drupal_set_page_content($conten
 function drupal_render_page($page) {
+  $main_content_display = &drupal_static('system_main_content_added', FALSE);

Thanks! Now it gets more obvious.

The only thing that worries me is the namespace and overall name of that static variable.

I don't have a better suggestion, but maybe something like 'drupal_render_page:output' or 'drupal_get_page_content' would work out better.

-9 days to code freeze. Better review yourself.

Introducing a new tag for feature freeze: API clean-up.

This looks ready to fly now, but we have 13 exceptions...

afaics it is due to #469242: Move <head> outside page.tpl.php

thanks for that ;)

I thought this patch would work.

But it doesn't work for me when experiencing this bug and applying the patch I still get:

Yeah, that's caused by the region.tpl.php patch that went in last night. Add a drupal_flush_all_caches() to index.php and reload twice.

locally, i'm only getting 3 exceptions, before the SystemMainContentFallback->setUp() phase, so i don't think this patch is the culprit.

I'm requesting a retest!

I'm unable to replicate these failures... is anyone else able to replicate them?

Dashboard module now requires Block module, so the test fails to disable Block module only.

finally managed to replicate the issue i was having locally, and fixed the issues of this patch.

botty is happy again, back to RTBC that is.

Quick review, nothing major.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    // Create and login admin user

Missing dot.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    $this->assertFalse(module_exists('block'), t('block.module disabled.'));
...
+    $this->assertField('site_name', t('Admin interface still availble.'));

We can write 'Block module' instead of 'block.module'.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    $this->assertRaw('id="system-test-content"', t('Content handled by another module'));

Sometimes your status messages end with a trailing dot, sometimes not. It is fairly random.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    $this->assertText(t('Content to test main content fallback'), t("Main content still displayed."));

We can use single quotes here.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    // Fallback should trigger when another module
+    // indicates that it is not handling the content.

Wraps early.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    // Fallback should not trigger when another module is handling content.
+    // Note that this test ensures that no duplicate
+    // content gets created by the fallback.

Wrapping seems somewhat random.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    // Request a user* page and see if it is displayed

Missing trailing dot.

+++ modules/system/system.test	15 Oct 2009 22:32:53 -0000
@@ -999,7 +999,87 @@ class SystemBlockTestCase extends Drupal
+    $this->assertTrue(module_exists('block'), t('block.module re-enabled.'));

We can write 'Block module' instead of 'block.module'.

here you go!

note that the "weird" wrapping in the inline docs is due to things just going over the 80 chars width, and it just doesn't look right when having one or two words on a new line

no more fuzz, nothing else changed since #102

I went ahead and committed this, since the behaviour after this patch is *much* better than what we had before. However, unfortunately, it removes a feature that is arguably one of the coolest things about making content a block: you can no longer remove the "content" from a page entirely and print only blocks there (through the UI). If anyone has any bright ideas on how to retain both, I'm all ears, but saying "This is now Panels's job with this fancy override variable" is also an acceptable answer, I think.