Problem/Motivation

Update module only has a few functions left, let's deprecate them and move them.

  • _update_no_data
  • update_get_available
  • update_create_fetch_task
  • update_refresh
  • update_fetch_data
  • update_fetch_data_finished
  • _update_message_text
  • _update_project_status_sort
  • update_storage_clear

Steps to reproduce

Proposed resolution

  • update_get_available -> UpdateManager service
  • update_create_fetch_task -> deprecate, it already has \Drupal::service('update.processor')->createFetchTask
  • update_refresh -> deprecate, it already has \Drupal::service('update.manager')->refreshUpdateData()
  • update_fetch_data -> deprecate, it already has \Drupal::service('update.processor')->fetchData()
  • update_fetch_data_finished -> move to static on UpdateController
  • _update_no_data -> move to UpdateMessageTrait
  • _update_message_text -> move to UpdateMessageTrait
  • _update_project_status_sort -> inline in update requirements
  • update_storage_clear -> UpdateManager service

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3580703

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Parent issue: » #3566536: [meta] eliminate core .module files

nicxvan changed the visibility of the branch 3580703-deprecate-update.module-functions to hidden.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
dww’s picture

dww
we/he/they
commented
Related issues: +#3580705: Deprecate update.compare functions

x-posting this:

My initial concern with this plan (and its siblings) is we already have some weird services that are sort of incomplete and interact with each other in “interesting” ways. I wasn't around core for a few years when folks started OOP-ifying this module. I don't love any of how it’s currently split up. I'd love to take some time (which I don't have in the next ~3 weeks) to really think about it and come up with a more wholistic plan for how we should organize all this. I'd rather do that before we start introducing new services.

Maybe we need a "Modernize Update Status" meta or something. 😅 Or turn this issue into that.

Super briefly, take a look at core/modules/update/src/UpdateProcessorInterface.php

" * Processor of project update information."

And yet, nearly all the methods in there are really about fetching data. The only "processing" that happens is parsing the XML into a giant PHP array and putting it into the 'update_available_releases' key/value temp store.

Meanwhile, I wonder if instead of a new "UpdateStorageClear" service, should whatever service does/should populate those key/value storages have a method to clear them out?

nicxvan opened merge request !15943

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Needs review

This is ready for review.

I ended up organizing it slightly differently, I think this actually ended up in a pretty nice place.

One weird thing is I noticed some places were using update_available_release instead of update_available_releases.

I fixed that in the two places necessary.

This is green and I think it's actually good step towards cleaning up update!

nicxvan’s picture

nicxvan commented

Added suggestions for 11.5 deprecation since this almost certainly won't make it into 11.4

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

I addressed most of the feedback, but a bunch of tests broke, gotta track it down.

Wrong issue.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

So MR didn't apply cleanly with a little fuzziness

error: patch failed: core/modules/package_manager/tests/src/Kernel/ProjectInfoTest.php:191
error: core/modules/package_manager/tests/src/Kernel/ProjectInfoTest.php: patch does not apply

So rebased

Searched for _update_no_data = all instances are gone (1 entry in phpstan just noting)
update_get_available = all instances replaced
update_create_fetch_task = all instances replaced
update_refresh = all instances replaced
update_fetch_data = all instances replaced
update_fetch_data_finished = all instances replaced
_update_message_text = all instances replaced
_update_project_status_sort = all instances replaced (1 entry in phpstan just noting)
update_storage_clear = all instances replaced

All threads appear to be addressed. Going to mark.

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented

I sorted the list of procedural functions, the replacements are now in a table. For the change record and deprecation text in the MR all references to the replacements are as its fully qualified name.

nicxvan’s picture

nicxvan commented

I realized the batch callback was static so I converted it here: https://git.drupalcode.org/project/drupal/-/merge_requests/15943/diffs?c...

It was just replacing:
\Drupal::messenger() with $this->messenger()
\Drupal::translation()->formatPlural with $this->formatPlural
t( with $this->t

Since it's a controller and it already has the messenger and string translation traits.

Could use a +1, but not sure it's worth bumping out of RTBC.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs work

The diff looks OK to me but I'm more concerned about the operation there, that stores an instance of the updateManager object in the batch daata. For that one it's actually important to be called as a service.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Yes, good catch, I should have looked up a few lines.

This should be ready again, thanks!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Reviewed. A few comments for context for future reviewers and a few minor suggestions on docs, return types and how the services are accessed.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I addressed all comments. I had to revert the null default for a follow up.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

My feedback has been addressed, this was RTBC before.

  • catch committed 672e2c38 on 11.x 
    task: #3580703 Deprecate update.module functions

By: nicxvan
By: dww
By...

  • catch committed b0d6a194 on main 
    task: #3580703 Deprecate update.module functions

By: nicxvan
By: dww
By...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

With the package_manager code, there's still an open issue whether to use update status or not for that in #3408901: [policy, no patch] Decide if and when automatic updates should rely only on packagist data to determine installability of modules.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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