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:
#605292: propagate failure during batches in update manager
#602484: Fix the report page when authorize.php completes an update manager operation
#606190: Fix handling of database schema updates in update manager workflow
Comments
Comment #1
bfroehle CreditAttribution: bfroehle commentedComment #5
jludwig CreditAttribution: jludwig as a volunteer commentedIt looks like this issue is still valid all these years later.
The differences between update_authorize_install_batch_finished() and update_authorize_install_batch_finished() are just:
- the docblocks,
- _update_authorize_clear_update_status() being called in update_batch_finished upon success
- Text changes: Installation vs. Update in the page messages
- a block of code adding a task for running update.php in update_batch_finished()
Considering 47 lines of code are identical besides the text 'Installation' vs 'Update', I think it's worth moving that code to another function.
This is easy for the most part, but I'm not sure of an elegant way to handle the change minor text change for the page messages.
Comment #18
quietone CreditAttribution: quietone at PreviousNext commentedI 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.