Remove update.php number dropdowns

Ugh, the experience in that comment is horrifying. And, although the poster is not particularly… endearing, I can imagine a more lovable newbie making the same mistake in the update.php form.

Ok, so I was pontificating on a solution. IMO, the only people that need to get at the version pull-downs are developers. Maybe the solution is to make the “Select versions” fieldset hidden unless a dev_update_version variable is set to TRUE. Then we can delegate the switching of that var to contrib (like devel.module).

Or maybe just skip the whole update.php?op=selection page if dev_update_version == FALSE.

I would *really* like to keep the selection step, but it could just be a list of modules that require update with checkboxes. This way you could skip modules you don't want to update yet.

Also, I'm not very fond of the "add a checkbox for idiots" idea. I don't think that being "Idiot-friendly" should be a design goal, especially when it's at the cost of bothering everyone else.

Oh, and I don't really understand why this is marked as 'critical'. Since when every issue that has a "user experience factor" (meaning, more or less *every* issue) should be critical?

This is an important thing to solve, but there is no rush and we can safely discuss it and try several ideas.

Moving the dropdowns to devel module would be great. The update system does fully load modules so .install files can use all of the module's functions. If the continue button on the first page were a full form, devel.module can alter it. This would remove the selection page.

In an ideal world, we would not be asking users if they made a backup, we would be giving them a backup to download or automatically storing one on the server. But that is another issue.

We should provide better information about what will be updated. The first page should list what modules have updates, and maybe how many updates will be run for each theme. Checkboxes do not make sense, users should always update everything, anything else would leave you with a broken site.

I agree with drumm except for a minor disagreement that "everyone should update all at once.". Your site might be broken if you go in stages. But thats OK if you are just planning on running one update after another. Running updates one at a time helps isolate problems. Anyway, removing a page is a big win.

If there are checkboxes, then the update finished page would need to facilitate completing the upgrade. Remember, if we do offer checkboxes, we are asking people of all skill levels to understand what they do and how to use them.

General users should be able to simply update without needing to make any choices and the update should just work. This is dependent on module writers making updates that work.

More detailed error messages for reporting bugs and debugging would help with isolating bugs, with or without checkboxes. If the UI and underlying update code are good enough, then the checkboxes are not needed.

There are lot of conflicting and good ideas in this thread. I've implemented a bunch of them. I consider this patch to be a step in the right direction, and not the ultimate solution. Those are for follow-up patches which I will submit, along with anyone else who cares to jump in.

• I did not combine the selection page with the initial info page. Thats because I intend to build out the selection page so that it shows a description for each pending update. That description will come from an upcoming patch introduces hook_update_info(). Large custom sites might use update descriptions as 'release manager' notes. It would be perfectly legitimate to have an update that only carries a description like go change php.ini to blah. These notes formalize rollout steps that can't be automated.
• The selection page now lists pending updates grouped by module. If you have none, there no longer is confusing help text. If you do, all you can do is press Update. No checkboxes - see drumm's fine comments above.
• There is no functional change here - just UI. The dropdowns are gone which is good because their meaning was very confusing. The schema version was always set as if everything ran no matter how what you picked in the dropdown.

This time, without another patch mixed in.

subscribe

Well, that was easy. I am now showing update descriptions for all pending updates. The descriptions are automatically derived from the Doxygen for each update function. I think this is our very best chance at getting developers to bother with descriptions. This is low a level of effort as we can get. We also benefit in that we automatically get descriptions for all past updates for free. This is how we have always documented updates.

Here is a screenshot. In order to duplicate this on your system, temporarily edit the record for system.module in the system table so its schema_version reads 7000. Then browse to the selection page.

I think this is a significant improvement. Not everyone will want to see all the techno lingo though. I would support to hide these details by default, but to allow people to see them if they choose to. It looks like a fieldset is in order.

Also note that the last line of your patch has a 'AAUse' which looks like a typo.

One, awesome usage of PHP5 with reflectionfunction. I wonder whether API module needs to open in that direction

Two, agreed with Dries, hide them in a collapsible fiedlset

Three, @Dries the error is on your end, there are not even two adjacent a letters in the patch.

Now using collapsible fieldset, with indication of how many updates lie within. I also renamed the step to 'Review updates' instead of 'Select updates'. Finally, I changed the button text to 'Apply pending updates' and removed redundant help text as a result.

The 'AA' string was in the last screenshot but not in the patch. I attach a new screenshot. I set to RTBC based on Dries' and chx reviews.

@chx - i mentioned reflection to drumm for api.module a long time ago and he rightfully said that api.module can't use reflection because it would have to load untrusted code as PHP. It currently avoids that with its regex strategy.

one collapsible fieldset per module (or package?) would look nicer than one huge fieldset..

If we are trying to simplify the page for the admin that does not care about technobabble, then one fieldset is best. If the committers want multiple fieldsets, I will implement that.

(off-topic, @chx if we have our own parser we can also more-easily parse CSS and JS. However, it does need an overhaul, which I have been thinking about.)

I am traveling and have not had a chance to fully review this. I would like to point out that a fieldset is not necessarily the best UI element to use, since there are no fields in it. But it is the best we have now, and it should be used.

This patch is a good first step to streamlining our update UI.

+1 for a single fieldset. The only thing I don't like about this patch is that the descriptions come from the Doxygen comments. Both serve two different purposes, IMO. It restricts how we can use Doxygen comments in the code.

I originally removed hook_update_info() to make development easier. There is no good reason to make developers list a bunch of functions when you can simply define the functions. That did require sacrificing the descriptions.

Hopefully, updates just work and there are good release notes. I think reading well-written release notes would be more useful than potentially-technical descriptions of individual database updates.

I would certainly not make this an option. Figure out the UI and stick with it. (Developers can have additional UI for debugging.)

Committed to CVS HEAD.

Well, sometimes (although very seldom) a end-user need to update again an update that Drupal thinks it did correctly. It happened to me twice. So using the update.php and being able to re apply an update to the database saved my day. This occured using Drupal 4.7.x, though.

So, with Drupal 7, can we always be sure that an update was made properly? If yes, indeed showing drop down numbers can be confusing. But if Drupal 7 cannot guarantee that an update has been properly applied, then we still need a way for the end-user to re apply an update.

subscribe

Well, there's definitely another issue that updates ought not to flag as completed unless they actually complete fully. Apparently this is difficult, though... I was talking to David Strauss and he said that Postgresql is the only database system that supports rollbacks on DDL (CREATE, DROP, ALTER, etc.).

I tried looking for an issue about this but can't seem to find one in 30 seconds. It'd be nice to have some sort of fallback procedure though (separate issue) with the addition of this code, so that Drupal doesn't ever skip running an update if there are problems. Maybe keeping track of what statements were already run and if there are SQL errors generated about the fact that they've already been performed, ignore them? Hm.

@webchick - this was a UI only patch. The old code also moved all modules to latest schema_version regardless of the value of the dropdown. So this patch made the UI consistent with the submit handling.

If we can't put DDL in a transaction, then perhaps we actually integrate an optional DB backup to the updater and then rollback if we detect a problem. Separate issue.

Right. :) But I'm saying that this UI patch now makes it impossible to re-run updates if they fail (unless you have devel module, which most end-users do not), so we need to address that behaviour of skipping the number forward if there were failures during the update.

And yes, separate issue.

RANT: With all these fancy unit tests, apparently nobody tests patches by running the code anymore.

This patch breaks updates, try to add

function system_update_7011() {
$ret = array();
return $ret;
}

to system.install and observe.

If you want this fixed, it would be helpful to post an actual error message. I'm not going to try out "experiments".

There was no helpful error message unfortunately, otherwise I'd have dug deeper. Just WSOD.

Refusing to try out a rather simple test case is rather annoying.

Funny one: the value of the submit button was changed to 'Apply pending updates'. That's why the $op in the switch case did not reach 'Update'. Attached patch changes the case.

changing status

Should that be wrapped in st()?

@webchick - none of update.ph is wrapped in st() or t() and thats deliberate - see the change history if curious.

Ok then!

Simple enough change; committed.

Since this patch has gone in people report that no new updates are running.

It seems that updates from other modules do not get found or scheduled to run unless system module has an update.

I'm sorry to bump this issue again, but this patch broke updates that are not in system.install. In other words update.php no longer performs modulename_update_N() for modules when system has no updates.

Attached is a small patch, but please review it thoroughly, specifically Moshe since he is the original author, and he might be able to explain why this was implemented in this way.

The problem lies in the lines around 259:

if (count($form) == 1 && $form['start']['system'] == array()) {
  // We don't have updates :(
} elseif {
  // We have updates :)
}

count($form) always is 1, because of how $form is built (everything goes into $form['start']). Therefore, if there are no updates in system.install $form['start']['system'] == array() will always be true as well (because of row 215), and thus the system will claim that there are no updates to be run, despite pending updates for other modules.

I've fixed this by using the $all array, which contains all pending updates. If it is empty, there are no updates to perform, otherwise there are.

BTW, it seems we need (more) testing for the update.php.

the patch looks quite a bit more sensible than what i did. sorry about that. RTBC indeed.

Committed. Hopefully this is actually fixed now. :D

I added this to the upgrade docs.