Update module uses incorrect variable for maintenance mode [#958456]

I am updating a module in Drupal 7 using the UI provided at Administer→Reports→Available updates (admin/reports/updates). Before I did my updates, I went in and manually set my site into maintenance mode. During the update process I saw the screen that said I should set maintenance mode and just left it checked since I was already in maintenance mode I figured it didn't matter. After I ran the update (for token in this instance) Drupal gave me a message that states: "Update was completed successfully. Your site has been taken out of maintenance mode." But my site is still in maintenance mode as I browse around. This is *really* confusing so hopefully we could sort that out so that it actually checks if I'm actually in maintenance mode and provide an accurate message.

I'm not sure if it just assuming I did maint mode during the update process instead of on my own and so it thinks it unset it even when it didn't or what. No time to dig into the code right now.

Comment	File	Size	Author
#9	958456-9.update_manager_maintenance_mode_variables.patch	2.77 KB	dww
#7	update.authorize.patch	1.33 KB	rschwab
#7	update.manager.patch	918 bytes	rschwab
#4	maintfix.patch	517 bytes	rschwab
#3	maintfix.patch	528 bytes	rschwab
#1	maintfix.patch	517 bytes	rschwab

Comments

Comment #1

rschwab commented 14 November 2010 at 19:13

Status:

Active

» Needs review

Status	File	Size
new	maintfix.patch	517 bytes

I think this should fix it.

Comment #2

14 November 2010 at 19:30

Status:

Needs review

» Needs work

The last submitted patch, maintfix.patch, failed testing.

Comment #3

rschwab commented 14 November 2010 at 19:32

Status:

Needs work

» Needs review

Status	File	Size
new	maintfix.patch	528 bytes

Oops

Comment #4

rschwab commented 14 November 2010 at 19:50

Status	File	Size
new	maintfix.patch	517 bytes

Yikes my noob is showing. Last try before I let a pro take it home.

Comment #5

14 November 2010 at 20:00

Status:

Needs review

» Needs work

The last submitted patch, maintfix.patch, failed testing.

Comment #6

sivaji_ganesh_jojodae commented 14 November 2010 at 20:06

Title:	Udpating a module incorrectly states I've gone back online	» Update module uses incorrect variable for maintenance mode
Version:	7.0-beta2	» 7.x-dev
Status:	Needs work	» Needs review

As a part of reviewing above patch I ran a grep and noticed that update module uses variable 'site_offline' where it need to be 'maintentance_mode'.

./modules/update/update.authorize.inc:179:  $offline = variable_get('site_offline', FALSE);
./modules/update/update.authorize.inc:186:      variable_set('site_offline', FALSE);
./modules/update/update.authorize.inc:236:  $offline = variable_get('site_offline', FALSE);
./modules/update/update.authorize.inc:238:    variable_set('site_offline', FALSE);
./modules/update/update.manager.inc:360:  $form['site_offline'] = array(
./modules/update/update.manager.inc:389:  if ($form_state['values']['site_offline'] == TRUE) {
./modules/update/update.manager.inc:390:    variable_set('site_offline', TRUE);

Changed title to reflect the bug. The patch in #4 needs work.

Comment #7

rschwab commented 15 November 2010 at 05:04

Status	File	Size
new	update.manager.patch	918 bytes
new	update.authorize.patch	1.33 KB

Thank you Sivaji! I've changed the variables in all the spots you found. Trying again.

Comment #8

dww

we/he/they

commented 17 November 2010 at 15:59

Status:	Needs review	» Needs work
Issue tags:		+Update manager

Ahh, so #201415: Add permission to access site in maintenance mode changed this before the Update manager went into core, and then it was never noticed during #538660: Move update manager upgrade process into new authorize.php file (and make it actually work). There's a lot of discussion about the way Update manager handles maintenance mode at #606190: Fix handling of database schema updates in update manager workflow but we should at least fix it so it's using the right variables for what it's trying to do now (even if that's not what we want eventually).

So, this looks good, other than we need to put all the related changes from a single issue together into a single patch, not separate patch files for each file touched by the issue.