Posted by timmillwood on April 1, 2009 at 9:19pm
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I would like to see an easy way to change the maintenance theme
With dedicated maintenance themes like http://drupal.org/project/offline the user/developer shouldn't have to change settings.php to enable it, it should be on the themes admin page (admin/build/themes).
Comments
#1
Maintenance theme is also for when database is not working. In that case, the only way to set variable is via settings.php. There is no way around this.
#2
Yes, good point.
:)
#3
This came up in discussion at #547068: D7UX: use Seven for installation / updates as something that might very well be good to do.
Even if we can't do it for the case where the database is unavailable, we can still expose this setting in the UI for the case where the site is deliberately put in maintenance mode. That seems like it might be better than nothing.
The attached patch is a start at implementing this. For now, it just lists the themes and let you choose one (and does not provide an explicit "always use the site default theme" option). It seems to work fine.
Of course, it would extra awesome if this could work when the database is unavailable also. It's maybe not actually impossible to make that work, just really difficult. And maybe there's a shortcut or something... store it in SQLite? :)
#4
subscribe, will test later
I take it we can't write changes to settings.php after install?
#5
Thank you.
#6
This can't be done in the theme layer? I don't get why we need another option for this?
#7
Drupal supports three different themes out of the box:
- The default theme; displayed normally.
- The admin theme; displayed on administrative pages.
- The maintenance theme; only displayed to site visitors when the site is in maintenance mode or otherwise inaccessible.
These themes are completely independent from each other. The system automatically chooses the proper theme for the current page, and the "theme layer" depends on that decision, i.e. the theme layer just reacts for the theme for the current page. Or in other words: This selection happens before the theme layer takes any action.
#8
This seems like an impending Drupal WTF. We give the user an option in the UI, then there's also the same option in settings.php. Which one takes precedence? If the one in the UI, then I foresee a lot of "I changed it in settings.php and nothing happened" and "Even though I selected a different theme in the UI, Minnelli still popped up when my DB went down!" questions in the forum/IRC.
At the very least, we need to add some good comments above the option in settings.php to describe the change, and maybe even link to a docs page above the option in the UI.
#9
Back to CNR to discuss.
#10
Configuration values in settings.php always take precedence. This is not limited to the maintenance theme, it applies to all system variables.
#11
Most system variables in settings.php don't also have a redundant-but-contradictory UI, so I still think comments are needed in settings.php. Would like to get a 3rd opinion, though, so CNR it is.
#12
so is this the theme for
a) install.php
b) update.php
c) site offline
or all?
(c) is easy in D6, but (a) and (b) is impossible in D6. see: #553662: Don't hard code minelli as install & update theme
From the patch it seems like it is still for (c) only, but I should probably test it.
#13
Maybe this would constitute too much of an "API change" at this point, but we could consider just making them separate variables? The one that you set via the UI would control the behavior when your site is in maintenance mode. The one that you set in settings.php would be a totally different variable that only controls the behavior when your database in inaccessible.
#14
@David_Rothstein: That might be the only reasonable choice. Based on sun's #10, that means that if it is set in settings.php, then the maintenance theme won't change no matter what you choose in the UI, which is another obvious WTF.
#15
GUYS.
This is the EXPECTED behavior for ALL system variables. It was this way since like forever, and it won't be changed here.
If this behavior is not documented enough, then we need to document it better. But that has nothing to do with this issue.
And of course, the system has to choose any theme in case the DB is down (and UI-configured variables aren't accessible). But that's all EXPECTED. It. is. the. default.
#16
Where else do we have a UI for something that, if already set in settings.php, won't actually do anything?
At the VERY least, we need to alert the user that it won't do anything.
#17
Everywhere.
Please uncomment the following line in your settings.php (around line 259):
$conf = array(# 'site_name' => 'My Drupal site',
Afterwards, try to change your site's name. Nada.
Again, this is the expected and _wanted_ behavior.
#18
@sun: Can you please stop setting this back to RTBC while people are still discussing?
It seems like there are a few legitimate concerns here:
1. We're adding another choice to the themes page, and it's not clear what the difference is between this, a regular theme, and an admin theme. This is adding extra complexity that the user has to understand. The extra choice is especially puzzling because I would say 99.9999% of the time, you actually want the site's front-end theme to be the same theme shown during maintenance, so that means re-selecting it in two places when you change the default theme.
2. Because when most people think maintenance theme, they think "ALL of the times when my website is inaccessible" which includes "MySQL is too busy" as well as "I'm taking the site down for updates." It would be a nasty surprise for people to choose "Custom Theme" as the maintenance theme option in the UI, only to have it show Garland in certain circumstances.
3. So to combat that, most people who care about this issue will set the 'maintenance_theme' variable in settings.php. but that means that this setting does nothing (though as does, as sun rightly points out, any variable that's hard-coded in settings.php). But this one seems particularly curious to offer as a setting since in order for it to behave as you really want you have to set this settings.php variable, making this setting totally useless.
Is there a reason we can't just variable_get('maintenance_theme', variable_get('default_theme', 'Garland'))?
#19
The difference here is that you probably won't uncomment stuff like that in settings.php unless you're already having a problem (so the WTF is rarely seen), whereas you're encouraged to set your own maintenance theme in settings.php (which would stay true even after this patch is committed because it's obviously the only way to set a custom theme for DB errors, etc.).
Edit: whoops, looks like I just repeated what webchick said (point #3)
#20
Here! Here!
#21
Hm, yeah, I mentioned this possibility in the other issue and suggested that changing it to that would be blocked on other things (for example, getting rid of Minnelli) - but actually I guess that's not true .... probably no one really cares the difference between whether their maintenance theme is Minnelli vs Garland anyway :)
Doing this would not, however, solve the original use case of the present issue. But it still probably makes sense to do :)
#22
Hm, well actually, just making that change would mean that any site which uses a theme that does not define its own maintenance.tpl.php file would basically wind up having its maintenance page look like Stark? That seems kind of unfortunate (and could be mitigated by also providing a UI for switching away from the default).
#23
Seems like that would solve the problem of the UI and settings.php having a different maintenance theme set, but it would still bork when the database is down (since variable_get obviously wouldn't work).
Hmm, what happens if settings.php tries a variable_get when the DB is down? Does it use the default case (in the example, Garland), or does it error out completely?
#24
variable_get() actually should work fine when the database is down. If the variable in question is defined in the $conf array in settings.php, it uses that. If not, it falls back on the provided default (i.e., 'garland' in the above example).
#25
Good. So now we can make settings.php and the UI be in sync with each other. That just leaves us with the problem of it still serving up Garland when the DB is down (assuming it hasn't been changed in settings.php), right? If so, then that's a problem I'm personally willing to put up with, as long as we document it in settings.php.
Anyone else?
#26
Actually, looks like JohnAlbin's tacking that issue in #637414: Clean up Seven theme's maintenance page for use as db-offline default.
So I guess we can use the patch in #3 along with the change to settings.php and call it a day?
#27
I don't get why patch #3 is desirable, really. Why offer a third choice to a user that needs to be synchronized with their default theme choice?
#28
Um, no.
variable_get('maintenance_theme', 'minnelli')would be used (i.e. the hard-coded Minnelli.)Given that the proposed UI would only work in #3 above, I have to ask: How often would a site admin want a theme different then their currently selected default theme? Not often.
Yes, but I would argue that a theme that doesn't work with the default modules/system/maintenance_page.tpl.php needs its own copy of that template. And if it doesn't, then that's a bug with that theme. Put more plainly: it should be a requirement that all themes be designed to work in maintenance mode. We could make that plain in the D7 upgrade docs.
Here's the much simpler patch suggested by webchick. We use the user-selected default theme (which can be overridden in settings.php) as the maintenance theme. If there isn't one (or the db is down), we use Minnelli.
#29
The last submitted patch failed testing.
#30
I agree that maintenance-page.tpl.php should be required for all themes.
However, if we do this, then we do it properly.
#31
Fixing the test.
#32
@sun, there's a difference between requiring that themes work in maintenance mode (as JohnAlbin suggested) and requiring that they include their own maintenance-page.tpl.php. I'd be in favor of the former, but not the latter, because the default maintenance page template is often fine.
#33
Somehow, that doesn't make sense to me. The whole point is that there is no reasonable default. And because there is none, we hard-code Minnelli/Garland/Seven. The latest patch at least tries to work around the technical limitation by still accounting for the configuration in settings.php when the database is down.
#34
By default, I mean modules/system/maintenance_page.tpl.php (or whatever), not Garland or Minnelli.
What I mean is, let's default the maintenance page to the active theme, but don't require that the active theme override that default template, because in many cases, the default maintenance page template + some css in the current theme is all you need. (aka JohnAlbin's suggestion)
#35
looks like the uhhh-wtf-issue from #63 in #547068: D7UX: use Seven for installation / updates got re-introduced somehow
got this in a fresh checkout today
it seems to have been caused by #639354: Spacing between horizontal radio buttons too crampy but I wouldn't say that "broke" it, it was already broken and that just showed it
making
body.in-maintenance #contentfloat (left or right, doesn't rly matter, but I'd say left makes more sense) fixes it without causing any repercussion as far as I can tellalso posting this in #637414: Clean up Seven theme's maintenance page for use as db-offline default
oh, looks like #641810: "Select installation profile" page looks weird with custom profiles tries to do exactly that
#36
I want my maintenance theme to be the same as my normal theme. Since all variable must be set in settings.php, I had to put some logic in there to see if we are in maintenance mode and use one set of values, else use another set for normal.
#37
This is an important issue if we want real distributions.
changes:
- added the same tpl selection logic to install theme
- minnelli removed from the patch, because as I see it is not part of Drupal any more
screenshot: Drupal install with custom theme
#38
RTBC if bot passes.
#39
#40
The patch in #37 is nearly perfect. But its missing a critical bit from the patch in #31, IMO. Can we add this back in?
@@ -46,7 +46,9 @@ function _drupal_maintenance_theme() {
drupal_load('module', 'system');
}
- $custom_theme = variable_get('maintenance_theme', 'garland');
+ // We use the default theme as the maintenance theme. If a default theme
+ // isn't specified in the database or in settings.php, we use Garland.
+ $custom_theme = variable_get('maintenance_theme', variable_get('theme_default', 'garland'));
}
$themes = list_themes();
That way the default theme is used as the maintenance theme, which is what the user expects when the site is in maintenance mode.
Also, I've previously said “it's a bug if a theme doesn't have a maintenance-page.tpl.php". But I should amend that. It's a bug if a theme has a page.tpl.php, but doesn't have a maintenance-page.tpl.php. For example, Stark doesn't need either tpls. But the patch will fail if Stark is the maintenance theme. To fix that, we just need to add another path to search in our $base_themes array. By adding one line of code after we initialize $base_themes; like so:
<?php$base_themes = $base_theme_info;
// Make sure a maintenance-page.tpl.php is always found.
$base_themes[] = 'modules/system';
while (!file_exists($theme_path . '/maintenance-page.tpl.php') && $base_theme = array_shift($base_themes)) {
$theme_path = dirname($base_theme->uri);
}
?>
#41
I fully agree with John, nice improvement.
#42
/me subscribes. We need this!!!!
#43
Committed to HEAD. Thanks!
#44
Automatically closed -- issue fixed for 2 weeks with no activity.