MODULE_preprocess_maintenance_page functions are not called
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | JohnAlbin |
| Status: | closed |
| Issue tags: | needs backport to D6, Quick fix |
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.
#1
Please review! :-)
#2
Actually, the functions do register, but the normal theme registry isn't used during off-line mode.
#3
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.
#4
after talking with JohnAlbin I get it now. when the db is active there are still issues.
#5
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).#6
Its just an additional if statement. Anyone for a review? Bueller?
#7
It was hard to test this...
I find this useful because if
<?phpmodule_list(TRUE, ...
?>
#8
This fix is small and useful so there's a reason to backport
#9
The last submitted patch failed testing.
#10
The bot was broken. Re-test, please!
#11
Now that the bot is happy again… RTBC per comment #7.
Thanks for the review, Andy! :-)
#12
The last submitted patch failed testing.
#13
what the hey? Why does it pass and then later fail? The patch still applies. Testing again…
#14
The last submitted patch failed testing.
#15
Bot was broken
#16
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?
#17
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 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. :-(
#18
Bumping back to RTBC to see if webchick approves of the new comment.
#19
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.
#20
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.
#21
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:
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. ;-)
#22
Yay, thanks! That's much more clear. Committed to HEAD.
Marking down to 6.x.
#23
Re-rolled for D6.
#24
Committed to Drupal 6 too, thanks!
#25
Automatically closed -- issue fixed for 2 weeks with no activity.