Unify update_authorize_update_batch_finished() and update_authorize_install_batch_finished()?

Created on 21 October 2009, over 15 years ago

Updated 29 July 2023, over 1 year ago

In #538660-134: Move update manager upgrade process into new authorize.php file (and make it actually work) → I explained why update_authorize_update_batch_finished() and update_authorize_install_batch_finished() need to be separate functions, even though they're pretty similar:

Split the batch finish callbacks in update.authorize.inc into separate functions for the update and install cases. I know they look a bit copy+paste, but most of the lines of each function are messages to the user, and those need to be different in the two cases. Plus, batch API doesn't let you pass in your own arguments to your finished callback, so we'd have to use the SESSION to remember what operation we're doing. Anyway, fixed all the messages to actually reflect reality.

At #538660-150: Move update manager upgrade process into new authorize.php file (and make it actually work) → alexanderpas seems to have ignored my comment and wrote:

update_authorize_install_batch_finished() and update_authorize_update_batch_finished() seem to be having almost the same code, could we optimize that somehow.

I still stand by my previous comment, but we could consider trying to unify parts of these, especially as we work towards solving the following issues, all of which will potentially touch these functions:
🐛 propagate failure during batches in update manager Fixed
#602484: Fix the report page when authorize.php completes an update manager operation →
#606190: Fix handling of database schema updates in update manager workflow →

📌 Task

Status

Closed: won't fix

Version

11.0 🔥

Component

Update →

Last updated 11 days ago

Maintained by
🇺🇸United States @tedbow
🇺🇸United States @dww

Created by

🇺🇸United States dww

Live updates comments and jobs are added and updated live.

Incomplete comments

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

Comment over 1 year ago →
🇳🇿New Zealand quietone
I have taken a look at this today and the two functions are have more differences than suggested in #5. One refers to 'Files were added' and the other is 'Update was'. The if block conditions are a bit different as well. Also, the text will be translated so variables aren't helpful here.

Altogether, I don't think combining them will help readability of maintainability.

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/updat...
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/updat...

I am going to mark this a won't fix. I trust if dww or anyone disagrees they will leave a comment.

contrib.social Blog FAQ Discussions

Production build 0.71.5 2024