[UX] Updates: Maintenance mode enabled automatically, but never disabled

[jenlampton]

Description of the bug When I choose to update my site using the self-update feature (but maybe this happens with regular updates too, I haven't checked) step 2 is "Put your site into maintenance mode." but when I run the update my site enters into maintenance mode even when I don't want it to.

We either need to remove step 2 from this list, or honor the setting during updates.

Either way we need to turn off the setting at the end of updates if we turned it on at the beginning.

Steps To Reproduce To reproduce the behavior:

1. Visit update.php
2. Update core without setting the site into maintenance mode
3. After the update, confirm that your site IS in maintenance mode.

Actual behavior Site IS in maintenance mode, even though you didn't set it to be.

Expected behavior Site should not be in maintenance mode.

---

PR by @jenlampton: https://github.com/backdrop/backdrop/pull/2803

[jenlampton]

The function where we un-set maintenance mode is update_finished which never gets called when updating core.

Instead, we call installer_manager_download_batch_finished

No, actually that's not called after the core db update, I think it might be called after the files are copied, i haven't tested that.

[laryn]

Related (but different): https://github.com/backdrop/backdrop-issues/issues/3550

[jenlampton]

Hm, I'm going to test that PR and see if it fixes this too... edit: nope, still stuck in maint mode :(

[klonos]

@jenlampton you seem to be mixing the use of state_get() with config_get() in your PR + I recall coming across some similar maintenance mode weirdness while working on #2583. Please have a look at the comments I've left in the PR and try again when you get a chance.

[jenlampton]

I think Ive tracked down the problem. It's in installer_authorize_update_batch_finished(). That function takes the site out of maintenance mode BEFORE the updates have been run. You can tell because the message on the page before you run the updates says "Your site has been taken out of maintenance mode." I think this is messing with the other setting and unsetting of maintenance mode. I'm going to remove it and see what happens.

Sorry @klonos, that PR was just a quick stash of my code (not ready for testing) b/c I had a presentation to give (on updates, ha!) and didn't want to loose my train of thought. The config state thing was something I got from the other PR at https://github.com/backdrop/backdrop-issues/issues/3550 but hadn't fully incorporated, more of a note to myself to change that line.

[klonos]

No worries. Was just trying to be helpful 🙂

[jenlampton]

Okay - I've pushed a new PR that fixes the config/state issue as per https://github.com/backdrop/backdrop-issues/issues/3550, and removes the state setting in installer_authorize_update_batch_finished(). After my first test, this is working!

Testing it is a pain because you need to apply the patch before the update to get the right session var set, and then apply it again after the code is replaced (but before the updates have run) to get the un-setting of the session var right.

@klonos you'll like this - I fixed some of the strings that gave the user incorrect information about which step of the update we were on, and I added classes to make some of the links into buttons :)

I may need to split that out into separate PRs, but I was getting very confused as to which step we were on and what was happening... so I fixed it here for sanity :)

[klonos]

@jenlampton I don't see any value for the novice Backdrop user in changing the Ready to update to Ready to replace code. They don't understand code/files copy vs db updates, and I don't think that they need to.

[klonos]

Hey @jenlampton ...as also mentioned on Gitter, I took a look at this, and although things may seem to work as expected (I have not tested it as thoroughly as I'd like), I think that there's room for improvement in the logic. I have started working on it off of your branch on my local; just not ready yet, and looks like will need some effort 🤔

[jenlampton]

This process was terribly confusing for me because none of the screens told me what the system was actually doing on any given step. If someone who knows how updates work doesn't get it, there's no way we can expect someone who doesn't to follow along.

We can't only tell people we 'update' on every screen because they'll wonder why we need to update 10 times.

Neither new people or experts will understand 'Ready' on it's own, and we don't use that word anywhere else in Backdrop. I'd rather educate people while telling everyone something useful than confuse everyone. I'm open to other text to put there, but 'Ready' by itself isn't going to cut it. :(

[jenlampton]

I think that there's room for improvement in the logic.

Ah, this is different logic. I don't think we should make those changes here @klonos, can you please open a new issue?

[jenlampton]

I've updated the PR to address comments, and rebased to avoid merge conflicts.

[klonos]

Thanks @jenlampton ...still in the process of reviewing/testing this, but the PR sandbox is loading install.php instead of a sandbox site. Can you please close/re-open to refresh the sandbox?

I may come around re the "Ready to replace code" thing, but will need to test this before I decide.

[klonos]

...forgot to mention that having 3 ways to set/get maintenance mode (via config, via state, and via $_SESSION['maintenance_mode'] as well) may be the actual culprit here (or an additional one). If all these 3 ways are needed in various situations, then we should make sure that all of them are being updated at the same time. So, perhaps we need a couple of functions like system_site_maintenance_mode_enable() and system_site_maintenance_mode_disable() and/or a single system_site_maintenance_mode_toggle() one, and then use these in system_site_maintenance_mode_submit() and elsewhere.

[jenlampton]

PR rebased. Docs updated too.

having 3 ways to set/get maintenance mode may be the actual culprit here

@klonos the problem is that even though they are all named maintenance_mode they don't all mean the same thing.

The session variable will indicate the intended state (once updates are done) so that needs to be different from the current state. It's used for comparing current state to intended state, so the system can tell if we need to switch back.

I just did a grep through the code and couldn't find any instances of maintenance_mode in config.

but the PR sandbox is loading install.php instead of a sandbox site. Can you please close/re-open to refresh the sandbox?

Using the sandbox probably won't help you test this, but I have requested a new one nonetheless :)

[klonos]

I plan to test and review this soon and get it into 1.17.5 ...assigning to myself, so that it's on my radar.