Add permission to access site in maintenance mode

New features go to Drupal 7.

This shortcoming hit me today. Regular users (e.g. "Site Editors" audience) should be able to access the site in offline mode to prepare, review and update content. You really do not want to grant them the "administer site configuration" permission.

I'll work on a patch.

Here we go.

Once you encounter this issue, it is a true Drupal WTF issue.

So this is a permission?

permissions.png: See "Access site in maintenance mode" permission.

site-maintenance.png: Slightly adapted form element description.

I don't think we have any site offline tests, so we could probably use them now since we're adding this feature. And probably use update_sql() instead of DBTNG until we can figure out what to do with update_sql.

/me ducks

Yes, drupal_get_normal_path() is useless here - right 5 lines below plain $_GET['q'] is tested without it, @see http://api.drupal.org/api/function/_menu_site_is_offline/7

Re-rolled against latest HEAD. system_update_7021() was removed.

We don't have any site offline tests, so we could probably use a little test. I can write it myself shortly.

Sorry, I did not want to unset that term.

Given the plain, simple, killing concept in #394268: DIE update_sql() DIE!, I won't convert this query to update_sql() here. 1) It's a 1:1 copy of the existing system_update_7011(), and b) I'm confident that update_sql() will completely vanish.

For any reason, last patch did not go into test queue.

The description on site-maintenance is a bit overwhelming, what about : "Offline", will only allow users with "..."permission to acces your site.

The other descriptions seem fine to me, I havn't actually ran into this issue in any test or anything - but it sounds like a feature request that needs to be fixed.

@Bojhan: Parts of this message contain very valuable information. I could only see the following option:

When set to "Offline", only users with the "Access site in maintenance mode" <a href="@permissions-url">permission</a> will be able to access the site to perform maintenance; all other visitors will see the site offline message configured below. Authorized users can log in directly via the <a href="@user-login">user login</a> page.

- or - turn it into a simple checkbox and say:

[ ] Set site offline

When enabled, only users with the "Access site in maintenance mode" <a href="@permissions-url">permission</a> will be able to access the site to perform maintenance; all other visitors will see the site offline message configured below. Authorized users can log in directly via the <a href="@user-login">user login</a> page.

I'd prefer the latter. Radio buttons are the wrong UI element here.

Let's change:

        if (user_access('administer site configuration')) {
          drupal_set_message(t('Operating in offline mode. <a href="@url">Go online.</a>', array('@url' => url('admin/settings/site-maintenance'))), 'status', FALSE);
        }
        else {
          drupal_set_message(t('Operating in offline mode.'), 'status', FALSE);
        }

to:

        $offline_message = t('Operating in offline mode.');
        if (user_access('administer site configuration')) {
          $offline_message .= t('<a href="@go-online">Go online.</a>', array('@go-online' => url('admin/settings/site-maintenance'));
        }
        drupal_set_message($offline_message, 'status', FALSE);

That way we don't have to translate one part twice. :)

SystemSiteOfflineTestCase should just be SiteOfflineFunctionalTest as per the test guidelines.

+    $edit = array(
+      'name' => $this->user->name,
+      'pass' => $this->user->pass_raw,
+    );
+    $this->drupalPost(NULL, $edit, t('Log in'));

Should just be:
$this->drupalLogin($this->user); since it uses the path /user to log in

Can we just change all these assert(No)Text calls from
$this->assertNoText($offline_message, t('Site offline message not displayed.'));
to this (so much cleaner):
$this->assertNoText($offline_message);

And instead of starting with

// Set site offline.
    variable_set('site_offline', 1);

in the test, why not login the admin user, turn on maintenance mode in the test using drupalPost(), then at the end, turn it off using drupalPost() and make sure the site is no longer in maintenance mode.

+1 for checkbox and a little slimmer description.

Updated tests, code, UI, and everything.

Just did not alter the user login, because my intention was to specifically test whether the user gets the site maintenance mode message directly after login.

Oh, hm - I also left out the split into "Go online.". Because I'm not sure whether translators are able to grasp the context this way. Opinions?

Gábor?

Looks nice. The update function is really nice to have but wasn't strictly necessary, IMO.

Looking at the screenshot in #18 with fresh eyes, two things occured to me:

1. I think it would be good to rename 'Site maintenance' to 'Site offline mode' or something. The words 'site maintenance' are very generic, and to most website owners 'maintenance' is unlikely to be correlated with 'off-line'. I bet that for most people, revising content or tweaking taxonomy terms might also mean 'site maintenance' ...

2. We don't explain _why_ someone might want to take their website offline. That seems to be a bit of a problem.

I know, probably outside the scope of this patch but I figured I'd bring it up in the context of this patch. We could copy-paste it into a new issue if desired.

I accepted the scope creep in the first place, so I'll try to flesh this out with the usability team.

One other thought is that I'd really like to see this maintenance message for privileged users vanish. It should be the job of the theme to (visually) indicate that a site runs in offline mode - having a message on each and every page looks and feels plain ugly.

Regarding the term in question, I've thought about cutting down "site maintenance mode" and "site offline mode" straight into "offline mode". As in "Enable offline mode".

Though I'm not too sure whether "offline mode" is translatable into all languages.

And I'm also not sure why someone would want to enable the offline mode.

Given the last sentence, I'm no longer sure whether "[enable] offline mode" is really the way to go.

I've tried.

However, a discussion in IRC revealed that the term "offline mode" or even "site offline mode" is not really translatable. People understand and can translate "maintenance mode", and that's most often also the meaning of the translated term, because "offline" is an unknown term to many languages.

Additionally, if we would want to change that term consistently, then we would have to alter many more parts of Drupal.

IMHO, we do not have to explain why someone would want to set a site offline. Site administrators will already know when and why. For me, it would be more important to be able to choose the maintenance theme on this very form (which can only be defined via settings.php at the moment - another DrupalWTF ? ;).

Let's defer those topics to another (usability-focused) issue - if at all.

Reverting to needs review. Actually, I think it's RTBC, but I need someone else to confirm.

Re-rolled.

Of course, that last one could only fail. Some paths and strings were renamed elsewhere.

Well, it is a month later and update_sql() is still here. And it has had no progress in that month. I don't think it is OK to cut corners here because of a possible future patch ... I don't see the problem in renaming to offline mode as Dries suggests. Translators are welcome to call it maintenance mode in their language if thats most natural.

Sorry for not being clear in my last follow-up:

- "offline mode" resp. "site offline" seems to have been completely replaced by "maintenance mode" by some other patch that was committed in the meantime. So this patch (now) follows that new wording.

- As stated earlier, system_update_7022() is a 1:1 copy of the existing system_update_7011(), which does not use update_sql() either.

I noticed in 6.x that if you provide incorrect user credentials to login to your site in maintenance mode, you get no error, as Drupal will return the maintenance page instead of a 403. I've opened #461022: Incorrect login details rejected with no message when site is offline to mention this, but I'm not sure if it should be incorporated here. It is something that is understandable if you only have the site admin logging in whiloe in maintenance mode, but if you have a less technical savy user who may need to enter content while the site is offline, then they will need feedback if their credentials are incorrect.

Note: Dries's comment at #20 was made before #430342: Change "Site maintenance" to "Maintenance mode" was committed. Over there we decided on "Maintenance mode" as the name of this setting, because "Offline mode" means something different in most applications.

I would therefore rather not include the hunk dealing with re-labeling those things and switching it to a checkbox, because the people concerning themselves with labels are over at the other issue. This issue should deal with adding the permission and testing it to make sure it works, and that's it. Let's mind the kittens.

I'd rather not link to the permission page. If someone has "access site configuration" permissions and not "administer users" permissions (a common configuration), they're going to get a 403 clicking that link.

I'm actually not really sure what the guideline is on update functions, but if we're copy/pasting from another update function that got committed, it's probably fine to leave it.

+      'description' => t('Log in when the site is offline.'),

"Log in when the site is in maintenance mode." Let's keep the terminology consistent.

+    // Create an administrative user.
+    $this->user = $this->drupalCreateUser(array('access site in maintenance mode'));
+    // Create an administrative user.
+    $this->admin_user = $this->drupalCreateUser(array('administer site configuration', 'access site in maintenance mode'));

There's obviously a difference between these two users. The comments should not be identical.

+    $offline_message = 'Sorry, not online.';

t() this, probably.

Note that "Sorry, not online." is solely used within the test, so no t() required.

Can you please explain how the patch ballooned from 9K to 14K when we were supposed to be taking *out* stuff? ;)

Hello,

Can I use this patch with Drupal 6? If so, how exactly would I apply it? (Which directory to I save it to, etc.)

Thanks for your help!

I second the request for a D6 patch!

Re-roll against latest HEAD.

Fixed the system update number.

@webchick: The size of the patch naturally increased after adding the tests ;)

Fixed the tests as well. :)

Patch seems OK, just a some minor issues.

+    '#description' => t('When enabled, only users with the "Access site in maintenance mode" <a href="@permissions-url">permission</a> are able to access your site to perform maintenance; all other visitors see the maintenance mode message configured below. Authorized users can log in directly via the <a href="@user-login">user login</a> page.', array('@permissions-url' => url('admin/user/permissions'), '@user-login' => url('user'))),

This path admin/user/permissions no longer exists.

   drupal_set_title(t('Site offline'));

The tile shouldn't be t('site under maintenance'), for consistency?

- Added reference to the update function that removed the old variable.

The rest, seems OK.

Beer-o-mania starts in 9 days! Don't drink and patch.

yay, ready to fly then. :)

+++ modules/system/system.test	22 Aug 2009 17:20:57 -0000
@@ -648,6 +648,95 @@
+  /**
+   * Implement getInfo().
+   */
+  public static function getInfo() {
...
+  /**
+   * Implement setUp().
+   */
+  function setUp() {

These don't get PHPDoc because they're inherited from the base class.

+++ modules/system/system.test	22 Aug 2009 17:20:57 -0000
@@ -648,6 +648,95 @@
+  function testSiteOffline() {
+    // Set site offline.

After all the trouble you went to rename everything, including a hidden variable to "maintenance" we should rename the test stuff too. :)

I'm on crack. Are you, too?

Excellent. :) Committed to HEAD!

Okay... so what's a patch and how do you implement it?
Is there a stable version of this yet?

@chien_fu: This functionality will be part of Drupal 7. If you want to use it on production sites, you will have to wait for Drupal 7.0, otherwise you can test it out already (http://drupal.org/project/drupal). It will not be part of the Drupal 6 release series.

It's a bit disheartening that this will not be fixed for Drupal 6. Any reason why it can't be fixed in 6 as well?

http://drupal.org/node/618402

VERY simple fix for Drupal 6

42: drupal.maintenance.43.patch queued for re-testing.