If your module has a MODULE_preprocess_maintenance_page() function in order to modify the variables for the maintenance-page.tpl.php template file, that preprocess function won’t be called when the site is in off-line mode.

The reason for this is due to the way _drupal_maintenance_theme() force feeds a short list of modules to module_list() before theme('maintenance_page') is called in drupal_site_offline().

The reason it force feeds a module list containing just system and filter modules is that the database might be down, so the theme registry has to be built using just those base modules and the maintenance theme. But actually, if the database isn’t down, force feeding the short module list is unnecessary and prevents enabled modules (which still run during maintenance mode) from using MODULE_preprocess_maintenance_page().

The fix is just to check if the database is down before doing the force feeding.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

Version: 6.x-dev » 7.x-dev
Status: Active » Needs review
FileSize
1.19 KB
1.21 KB

Please review! :-)

JohnAlbin’s picture

Title: MODULE_preprocess_maintenance_page functions not registering » MODULE_preprocess_maintenance_page functions are not called

Actually, the functions do register, but the normal theme registry isn't used during off-line mode.

mfer’s picture

Not sure I like this method. Basically, it means that custom maintenance pages may not work when the database is down because we can't get to some of the preprocess functions.

Not sure what would be good to do though?

I want a registry cached in an sqlite file so it's really available when the main db is down.

mfer’s picture

after talking with JohnAlbin I get it now. when the db is active there are still issues.

JohnAlbin’s picture

When the db is down, its impossible to know which modules are enabled anyway, so we're SOL anyways; the current code works fine in this case.

But, if we wrap that bit of code in if (!db_is_active()), then it will start to work for db-online cases (which completely fail now).

JohnAlbin’s picture

Issue tags: +Quick fix

Its just an additional if statement. Anyone for a review? Bueller?

andypost’s picture

Status: Needs review » Reviewed & tested by the community

It was hard to test this...

I find this useful because if module_list(TRUE, ... so there's no way to refresh module list in case of db is active! so RTBC

andypost’s picture

Issue tags: +Needs backport to D6

This fix is small and useful so there's a reason to backport

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review

The bot was broken. Re-test, please!

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Now that the bot is happy again… RTBC per comment #7.

Thanks for the review, Andy! :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review

what the hey? Why does it pass and then later fail? The patch still applies. Testing again…

Status: Needs review » Needs work

The last submitted patch failed testing.

andypost’s picture

Status: Needs work » Needs review

Bot was broken

webchick’s picture

Status: Needs review » Needs work

Since this has confused at least 2-3 reviewers, including me, could we please document the code change so the next person to stumble across it doesn't file a bug report?

JohnAlbin’s picture

Code-wise, the only thing this patch adds a single if (!db_is_active()) which I thought was self-documenting: if (!db_is_active()) == if db is not active then do the stuff inside the if block.

I've re-read, webchick's "needs more documentation" comment over and over. And the only thing I can come up with is the original comment (which my patch did not modify) is confusing: “load module basics (needed for hook invokes)”.

So this new patch updates the comment to read:

If the database is down, module_list() is empty and we don't know what modules are enabled. So just load the modules needed for hook invokes.

If I'm wrong about what is confusing, please be more explicit. 2 months in the issue queue for an if statement! So much for “quick fix”.

Sending the patch to the end of the testing bot line. :-(

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Bumping back to RTBC to see if webchick approves of the new comment.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I think this comment "So just load the modules needed for hook invokes." needs to be expanded to mention _why_. For example: "Just load the modules needed for hook invokes so we can ...". When you do that, this patch it RTBC.

andypost’s picture

Just load the modules needed for hook invokes only if database is NOT active, else module list is not going to be limited to 2 modules if we have database active.

JohnAlbin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.27 KB

I think this comment "So just load the modules needed for hook invokes." needs to be expanded to mention _why_.

Well… I had no freakin' clue. Its not my language. It's Steven's comment; introduced in this patch: http://drupal.org/node/68926#comment-417158. And the code is Eaton’s; introduced in this earlier patch: http://drupal.org/node/68926#comment-417155 #68926: Installer for Drupal Core Then Earl copied that verbatim into _drupal_maintenance_theme(). So…

I tracked down Eaton and MerlinofChaos in IRC and got this explanation:

[the code] relate[s] to that 'bootstrap things juuuuust enough that we can do more stuff' phase

I seriously contemplated putting Eaton’s “juuuuust enough” in the code comment, but the new code comment is a quote from Earl.

Nobody can say I don't do my homework. ;-)

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Yay, thanks! That's much more clear. Committed to HEAD.

Marking down to 6.x.

JohnAlbin’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
1.29 KB

Re-rolled for D6.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6 too, thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D6, -Quick fix

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