Fix handling of database schema updates in update manager workflow

While we're at it, let's use this issue to remember what alexanderpas pointed out at #538660-150: Move update manager upgrade process into new authorize.php file (and make it actually work):

+++ modules/update/update.authorize.inc
@@ -0,0 +1,302 @@
+      variable_set('site_offline', FALSE);

"we shoudn't turn online when we were already offline before we started the process."

webchick's #2 basically covers this, but I wanted to record it here so it doesn't get lost/forgotten. Basically, we shouldn't be setting the site online automatically at all. We should just present a landing page with a UI to control the site off/online state, and let admins choose what to do...

Well, at the *very* least we shouldn't put the site online unless they put it offline as part of the update workflow.

Better yet, if we detect DB updates, we say: "Yo, your new updated code has DB updates to run, leaving your site offline until you deal with that." Sorry, but "go get coffee" isn't an excuse. ;) What are they going to do with DB updates? What if something went wrong? If they miss all our help text and messages, then at least core is going to keep reminding them the site is offline on every page load until they put it back online again...

Ideally, we run try to run the missing DB updates somehow, and then give them a final landing page. Perhaps at that point if they put the site offline in the first place, and everything completed successfully, we can automatically but the site back online again...

Actually, this is totally broken right now: e.g. it tells you you've got updates when you don't. Upgrading this to critical, since the update manager really doesn't work as intended and is likely to confuse and annoy site admins as long as this isn't fixed...

Ugh, while working on this, I ran into #683988: failure to clear the system_list entry from {cache_bootstrap} results in bogus results when you upgrade a module which makes it kind of hard to test this. :( But, here's a patch that *would* work if core actually looked for DB updates in the right place. At least this no longer tells you to run update.php for code updates that don't have DB schema updates to run. Instead of trying to figure this out separately for each module (which would require loading all of includes/install.inc during the batch in authorize.php, which I'd rather not do, plus, it's weird UI), we just do a single check for any missing DB updates on the authorize.php landing page. If this test actually worked, it would include the link to update.php and tell you to run it. ;)

We still don't properly handle the site offline stuff, but we're getting closer...

Well, duh. ;) Of course #5 isn't going to work, regardless of the bugs in #683988: failure to clear the system_list entry from {cache_bootstrap} results in bogus results when you upgrade a module, since authorize.php is running at a very reduced bootstrap level. Tee hee. So, that module_list() call is just giving us 'system' and 'user'. ;)

We still need to solve #683988, which is a mess, but this also needs more thought... Good thing I submitted #5 as 'needs work' in the first place. ;)

Grrr, even if we use module_list(TRUE) there to test all the enabled modules, since we still haven't done a full bootstrap, we're still doomed. drupal_get_schema_versions() continues to return FALSE, since no .install files are loaded. I honestly don't see any way to reconcile this functionality with the desire to keep authorize.php at a low bootstrap level so that the landing page doesn't completely blowup if a bogus install was executed. Not that authorize.php currently provides you any way to recover from such an error. :( Ugh.

Stepping back, we have a few options here:

A) Just remove all reference to update.php, since we can't (yet) reliably tell you if you're missing updates or not.

B) Always have either a link (or perhaps even a "next" button) that takes you to update.php with some help text that explains why that's a good idea. In this case, we should probably add something to the landing page when update.php completes to let you take your site back online at that point...

C) Actually load all the .install files so we can accurately tell if you're missing updates and only conditionally do (A) or (B). This sucks since it means if the update includes busted code, we die as soon as we hit the landing page, and lots of effort has gone into trying to avoid that so far.

D) Do some wonky parsing of .install files without actually loading/eval'ing any of that code. Probably a lot of bloat for not much benefit.

Based on IRC chat, webchick and I are leaning towards (B) for now, since it's simple and at at least fixes the existing UI bugs in here. Attaching a screenie of the current landing page for comparison (probably belongs over at #602484: Fix the report page when authorize.php completes an update manager operation but whatever, here's fine for now).

p.s. Oh, except I want to look at how update.php and update.inc handle this, since I think they manage to discover the pending updates without actually loading everything...

re: #9: drat, nope. update.php does the full bootstrap eventually. update_get_update_list() is sort of what we want, except it's relying on all the .install files being loaded, since it's calling drupal_get_schema_versions()... So, there's no happy option E (yet) that magically does everything we want and doesn't suck.

I'm not sure a "next" button is best, since the update.php landing page sort of starts over assuming you haven't done anything yet, and tells you to backup your code/db, take your site offline, etc. :( It's not really seamlessly integrated with the update manager workflow in an ideal way, which makes sense, since we need update.php to stand alone and work for sites using other code deployment tools like drush, etc.

So, here's a patch that at least removes the existing bogus handling of DB updates, and adds a generic "task" at the end. Simple patch and before/after screenshots. Note that the before screenshot is wrought with problems, even though it actually looks like it's doing what we want. It always displays that message for the alphabetically last module that was updated, regardless of if it's missing DB updates or not, and it clobbers the same messages from other modules that might actually have pending updates, etc. That stuff is all broken and needs to be fixed either at #602484: Fix the report page when authorize.php completes an update manager operation or #605292: propagate failure during batches in update manager.

Yes, this page is still terrible. We still desperately need a UI cleanup at #602484. But, this is an incremental step in the right direction, since at least we won't be lying about non-existant DB updates anymore.

Why are we sacrificing the good workflow, for no reason? ... Even given all the circumstances we should not sacrifice on this?

I tend to agree, except there are reasons (maybe not "good", but reasons):

- This is complicated to actually make it work on a technical level, without opening many cans of worms in how authorize.php and the update manager were built from the ground up (the fact that authorize.php doesn't load all of Drupal on purpose in case something is broken during the upgrade).

- This has been broken for months, and in spite of repeated attempts to solicit help working on it, no one's shown up to do anything about it.

- I couldn't personally plow through every possible update manager related issue on my own over the last few months, due to a combination of work and personal fires. It's rather miraculous how much I've managed to get done in this area, really.

- It's a few hours before the alpha1 release.

Given all that, in IRC last night we decided that something like #11 was the best we could hope for. If you can help make a compelling case that this is really a critical UI bug that needs to be fixed properly, such that either we delay alpha1 for it, or relax the string freeze in alpha2 for it, I'd be thrilled. But, I think you're going to have to be able to convince Dries directly of that. webchick's mind is clear on this...

Meanwhile, #602484: Fix the report page when authorize.php completes an update manager operation is still completely insane. It's not even valid HTML. ;) So, we *can* make some progress there if we had a clearer plan on exactly what we wanted that page to look like. Although, it really deeply depends on what we finally decide to do here. I'm a bit stuck on how to best proceed without feedback from Dries on what's possible and when.

I have to be mostly AFK for the next 5 hours or so. After that, I'll plow into these issues and hope for the best...

Thanks,
-Derek

I've already explained why "checking for database updates" is a very difficult problem on the final landing page of the update manager workflow on authorize.php after the code has been updated. Please (re)read comments #7 and #8 above. "Checking for newly added dependencies" is the same problem -- we'd have to load all the code we just updated to know if there are dependencies, which breaks authorize.php in case of a botched upgrade.

In terms of "always send them to update.php", when/how/where are we supposed to report the results of the code updates? If there's no final update manager landing page on authorize.php and we immediately redirect to update.php, either:

- We never provide any feedback to users about what happened with the code updates (ugh)

- We have to somehow carry around all that data and display it at the end of the update.php process

- When we first land on update.php, it prints you out the results from authorize.php and then asks you to click "continue" (or whatever) to begin the DB update process.

I don't see how the final option is much different than just adding a "Continue" button on the final authorize.php landing page that is really a link that takes you to start update.php, perhaps with something stashed in your session to remember that you're coming from update manager so that the welcome text doesn't tell you to do things you've already done (backup the DB, put site offline, etc).

I have no time in the near future to be working on this, so I'm unassigning myself. If someone wants to work on it, please knock yourselves out, but I can't drive this home right now. I'll just try to help answer questions and avoid wasted effort as people propose solutions and patches here.

Cheers,
-Derek

@sun: There's already a landing page in update manager before you even get to authorize.php suggesting you backup everything before you continue.

@Bojhan: This is a Hard Problem(tm), just like #606592: Allow updating core with the update manager. In the abstract, It'd Be Nice(tm) to fix this Properly(tm), but we'd need a lot more people really working on this to make that possible, or this would have to be the only thing I'm worrying about in Drupal (yeah, right). Believe me, I'm as much of a perfectionist as anyone, but simply asserting that something is critical doesn't make it easy to solve and doesn't mean resources magically fall from the sky to make it happen.

@catch: Exactly. ;)

Thanks,
-Derek

Re: "there has to be some identifier that there are database updates"

I don't know how to say this any other way: We. Can't. Know. This.

It's possible (sometimes even easy) to draw pictures for how we want the world to look. Yes, we all agree the best workflow would be for this link to conditionally appear if needed. However, there are deep, far-reaching technical limitations (which I've explained multiple times) that make this vision impossible without Very Big Changes(tm) which I suspect Drieschick aren't going to want to see happen this late in the D7 development cycle.

Bojhan, I understand there are UI/workflow bugs here. It's not perfect. But, we can't make this perfect without a lot more sustained effort on Update manager from a wider pool of people. For whatever reasons, other D7UX improvements like overlay got the vast bulk of the resources and attention, while the update manager was basically left as something a tiny group of us did in our spare time...

Keep in mind, it doesn't *hurt* to always suggest the user visit update.php, even if there aren't updates that need to be run. So, one solution here is to remove the other distracting links like "Administration pages", "Front page", etc.

If it's an update, we have one link to "Run database updates".

If it's a new install, we have one link to "Enable newly added modules".

This is still very do-able in D7, and would encourage people to always take the right next step in the workflow they're in the middle of.

@rschwab: Thanks for getting this going!

However, the patches at #50 need work for the following reasons:

A) In general, please include all changes in a single patch instead of separate patches for each file you touch.

B) These patches don't actually implement the proposal in #44. ;) Some distraction is removed, but we still need to add the main content we care about which is the correct next step in the workflow based on the current operation.

C) The change about "Enable newly added modules" belongs over at #937698: "Enable newly added modules in [Module Group]" is awkward, should be "Enable newly added modules" not here.

Yes, removing the extra links is part of my proposal in #44. Also, it's been so long that I don't remember and I don't have time to look at the code, but I'm pretty sure authorize.php remembers what operation you tried to do when you land on the final landing page. You shouldn't need to query the {system} table to figure out if you did a new install or an update.

@Bojhan: except when you're updating. Then "Administration pages" is just a distraction that's likely to get people to forget to visit update.php, which is the whole point of this issue. But, I can see how "Enable new stuff" isn't the only thing you'd want to do after installing something. And in that case, there's little harm in having the user do something we're not expecting. So, for install, maybe we want "Enable newly installed modules", "Install another module", and "View administration pages"?

I asked Bojhan about consistently using verbs and he said:

[12:17pm] Bojhan: dww: Yhea, we don't say View administration pages
[12:17pm] dww: okay.
[12:17pm] Bojhan: view adds nothing :)

*shrug*

So, we can still use "Administration pages" even though the other links will have verbs in the install case.

Anyway, I think #57 is good, we just need to re-add a next step link for "Administration pages" to the install case to address Bojhan's concerns.

Addresses Bojhan's concerns while fixing the major UI bug here in the update case. RTBC. Thanks, rschwab!

FYI: I just opened #1478288: D8: Fix handling of database schema updates in update manager workflow to see if we can do any better in D8. I don't have high hopes, since the problems in here are nasty, but at least there's a place to discuss it and try again...