This time, I examined the update.php results page properly, after another of my testing 5.x->6.x upgrades. I noticed that several updates claim to have no queries (or no action in more loose understanding) executed, which is either not true, or a reason to remove them. I looked at this closer, and found that:

- Several update functions were really empty (leftovers from further changes after they were introduced). Apparently, people got used to turn removed updates into so called "no-op" empty functions, but this makes no sense as I see it: The numbering of updates is NOT required to be continuous, there may well be a hole. So no need to have empty updates, hanging around on the update.php screens (results page especially). So I removed these, leaving just comments at their original places: system_update_6004(), system_update_6012(), system_update_6028().

- Additionally, system_update_6031() was a duplicate of system_update_6014(), so I removed it too.

- Other update functions in fact performed some tasks, but returned no messages. Although the exact queries executed are not available (due to nature of these functions), it's still useful to return some sensible message. This is not new: system_update_6017() already reports executed variable_set() calls and the like, system_update_6021() says "Relocated xxx existing items to the new menu system." instead of listing all the queries.

I think it's really necessary to add some messages to all updates, so that the update.php results page (copied to a clipboard, for example) may be later analyzed for troubleshooting purposes, to find what exactly happened, and where this or that change was introduced (i.e. on update, or somewhere else in code). Otherwise, we might as well nuke that page entirely, and only show failed queriers. Currently the update process silently hides such changes, as enabling a new module (dblog), or adding new filters to my existing input formats (HTML corrector), which is not nice. So I added such messages to system_update_6013(), system_update_6014(), system_update_6018(), system_update_6029(), and comment_update_6002().

- Along the way, I found one apparently broken message in system_update_6018(), where a plain string was returned instead of proper array structure, even using a t() call (which is generally not used in updates as I understand it).

- Also update.php was using a t() on one place (only one). I see that the update.php script is not translatable and uses no t() calls, so this should be consistent. Also using a t() there is not a good idea, because the locale module itself is a subject to updates, so if the script is ever going to be translatable, it should use something along the lines of st() as I see it. Additionally, there's no need to clutter the runtime translations database with update.php strings.

This change is not random from me: I often got a weird message at the very end of updates, when there was just 1 item left, and although I didn't track it down exactly (quite difficult in that exact location), I ran into this t() misuse on that very message. (And surprisingly, it seems to fix my problem, although it might be as well random move of the percentage with other changes in this patch.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JirkaRybka’s picture

Patch still applies, but needs some other set of eyes on it...

catch’s picture

No time to test right now, but this looks very sensible to me and I often wonder about those "no queries were executed" messages (although I can never be bothered to check either). All the docs and string changes are great.

Gábor Hojtsy’s picture

- I am not sure you actually looked into system.install in regards to t() usage, but I would suggest you look into all core module's update functions, some might use t() which is not right as you pointed out.
- Otherwise I am not sure having holes in the update function list is actually desired. I am not as comfortable as you with this change, but the other changes look good and logical.

JirkaRybka’s picture

- Fixing t() misuses was originally not my aim here, I only just fixed these along the way (where spotted one). Now I searched all core .install files:
* Found some more in book module updates and the menu migration update (fixed)
* Another exist in old 4.7->5 system updates (1005, 1010), but I'm quite unsure whether we are supposed to touch old updates for such reasons (I guess not, so not changed now). To elaborate more, system_update_1005() uses t() on strings stored into database then, so removing t() will most probably break that update. It worked well back on 5.x (and might on 6.x even), so this one should stay IMO. The system_update_1010() is clear misuse, but still not sure what to do here (if anything).
* For a plain grep there are lots of t() in all .install files, but these are Schema API field descriptions, so definitely out of scope here. I was always unsure whether t() is a good idea there (translation might be nice at display-time like watchdog, and also flooding the UI translations database with unnecessary strings), but again - out of scope here.

- Holes in update functions are in my opinion painless (?) But anyway, now attaching a patch with just the t() fixes, AND going to roll an alternative without update-removals too.

JirkaRybka’s picture

Alternative: Version without removals (so this one leaves the "no queries" updates in place, where it's really true). Also turns system_update_6031() into a traditional "no-op" (the duplicate update).

JirkaRybka’s picture

Applies with just a bit of offset now.

Gábor Hojtsy’s picture

Status: Needs review » Fixed

These changes now look good, so I committed them to 6.x. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.