Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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 CreditAttribution: Crell commentedDur, Dreditor...
Comment #2
ParisLiakos CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch.
Comment #14
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedThe patch is not correct. So changing the status to Active so that someone can pick this up.