Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Follow-up from #1987888: Convert update_manual_status() to a new style controller.
The controller conversion issue (above) moved all of the meaningful code of the update system into classes. However, those classes are still pretty darned big and godly. It could stand to have further refactoring to break it up into discrete, single-purpose services.
As none of those classes is really part of the public API of Drupal, this is neither beta blocking nor release blocking. It's still something we should do, though.
That is, I do not consider refactoring those classes entirely to be an API break; if the committers wish to disagree with me on that they're welcome to do so, but IMO this falls into the "not an API break, so safe for 8.1" category. If necessary we can quickly tag those classes to indicate that.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | break_up_update_system-2167779-12.patch | 87.32 KB | nitesh sethia |
| #6 | drupal-update-refactor-2167779-6.patch | 87.23 KB | zengenuity |
| #4 | drupal-core-method-renamings-2167779-4.patch | 8.17 KB | internetdevels |
Comments
Comment #1
Crell commentedDur, Dreditor...
Comment #2
ParisLiakos commentedi would like to see some method renamings here as well
eg
UpdateManagerInterface::fetchDataBatch()makes no sense to me..maybe
fetchDataUsingBatchor w/e, maybe justfetchData?UpdateManagerInterface::projectStorage()would be a lot better to be
getProjectStorageinsteadUpdateProcessorInterface::numberOfQueueItems()same
getNumberOfQueueItemsUpdateFetcherInterface::getFetchBaseUrl()hmm, thats very confusing :) maybe just
getBaseUrlComment #3
kim.pepperThose batch callbacks confuse the hell out of me.
Personally, I'd like to see UpdateProcessor::parseXml() moved to a separate UpdateInfoParserInterface.
Comment #4
internetdevels commentedI've tried to rename methods as it is mentioned in #2. I hope this may be useful, just ignore my patch if it is not :).
Comment #6
zengenuity commentedHere is a first attempt at refactoring these classes. I've added UpdateParserInterface and UpdateParser as suggested by @kim.pepper, and reorganized some of the functions between UpdateManager and UpdateProcessor to reduce external dependencies. I also integrated all of the functions from update.fetch.inc and update.compare.inc, and replaced them with deprecated stubs in those files.
Comment #7
jhedstromComment #8
deepakaryan1988Comment #9
deepakaryan1988Re-rolling patch which is in #6 but I think it's not correct.
I think it will needs to re-roll.
Will do after this patch.
Comment #10
deepakaryan1988Comment #12
nitesh sethia commentedRerolled the patch.
Comment #14
nitesh sethia commentedThe patch is not correct. So changing the status to Active so that someone can pick this up.
Comment #28
dww#3100110: Convert update_calculate_project_update_status() into a class would be a helpful start, without diving into everything mentioned here.
Thanks to #3491731: [META] Remove the ability to update modules and themes via authorize.php and friends, the surface area is now a lot smaller, but there's still probably some modernization and cleanup that can be done, so not closing this as "outdated" or "won't fix"...