Finish deprecating 'Update Manager' related code in Update Status

Minor comments.

Thanks! Applied suggestions. Leaving the thread about moving the test into a new class open to see if anyone else has a strong opinion.

Thanks to our new gitlab bug I can't drill down into the test failures.

There are no failures. The failure was the pipeline never started. Longwave and Drumm seem to have sorted it out.
I’ll push a no-op commit to hopefully trigger a new pipeline.

Yay, the pipeline really ran. However, there seem to be failures, after all. 😂 I’ll look more closely when I’m at my laptop.

Was calling the deprecated method from hook cron, which caused any tests that enable update.module and invoke cron to trigger deprecation notices. Removed that from cron, and now the pipeline is all green. Back to NR.

What is the full status of this initiative? Does this change need a CR?
If this piece is just clean up then I think we're ok, however, since we're deprecating things I think we need to notify people that disk cleanup won't happen anymore.

All the new deprecations reference the CR from the parent issue. If we’re happy and this lands, I’ll update that CR to mention these other deprecations. Assuming this ships in 11.2, seemed silly to use a separate CR for it. See the parent issue and linked CR for more.

Thanks!
-Derek

p.s. This specific disk cache cleanup method is only cleaning up a cache of tarballs downloaded for the deprecated and removed “Update Manager” stuff. So nothing would be populating this cache, so no need to garbage collect it.

12 answers my question, I thought that was true, but wanted confirmation.

To make #11 explicit, adding the tag. I hereby solemnly swear to make the updates once this is committed...

Added a remaining task, probably for release managers:

Decide if we should explicitly mark the allow_authorize_operations setting itself deprecated in sites/default/default.settings.php and core/assets/scaffold/files/default.settings.php

Do you want to just do it to be safe? Or open a follow up? I'm hesitant to mark this as ready until then.

Added a few more remaining tasks for decisions, and tagging for release manager review.

Per @berdir in Slack, 📌 Convert Convert Template Preprocess hooks in core/includes Active is related. We also forgot the authorize-report template and preprocess. I pushed 2 commits for those, 1 to properly deprecate the template and preprocess, and another to add @deprecated comments to authorize-report.twig.html files (both system and stable9). Can revert the last one if that's not desirable for some reason.

Add authorize-report to summary, and move the list of deprecations to the API changes section.

I find it better to file specific CR as all this changes make sense and disruptive enough to reference for developers, moreover summary is perfect!

Regarding point #2. Having a change record associated with the single issue where the change was made makes sense. It is then easier for anyone who wants to know more to read that one issue instead of searching several issues. The text of the change record can always add a reference to other issues.

That's 2 votes in favor of a new CR for this issue. Created https://www.drupal.org/node/3522119 → and moved all the deprecations to point there. Removing the tag.

Noticed I also missed _update_manager_unique_identifier(). That's not used for the fetch URL to be able to aggregate anonymous usage stats per site. It's only used for the name of the disk cache subdirectories.

_update_manager_unique_identifier doesn't need to be deprecated though, it starts with an underscore.

Maybe we need an issue to track everything that should be deleted outside the deprecations?

For the final point 3. I think we do want a post update hook but that would be part of drupal 12, so maybe a follow up for that, you want it to run after you cannot possibly add new files there.

Re #23: see 📌 [12.x] Remove all legacy code related to authorize.php and FileTransfer Postponed from 2 parent issues up. Yeah, we probably don’t *need* to deprecate all of this, but it seems safer to be complete at this point.

Yes a follow-up for #24 for once Drupal 12 is open sounds good.

I'm not convinced the post_update() needs to be in 12.0. We've already removed all the UI that could trigger any of the code paths that put files in these temp directories. Some of that shipped in a previous release, the rest will be in 11.2.0. Can't we have a post_update() in 11.2 itself that removes these directories? Why leave them around for years? We mention cleaning them out at the CR. Probably also would in release notes. Why wait another major release for a post_update() to automate that? Once a site is running 11.2.0 or higher, nothing could be populating these directories unless we now believe there are contribs or custom code out there using any of this plumbing. Seems like those folks are on their own, and can be responsible for removing the directories again when they're really done.

But core will already be done the moment we're on 11.2.0. So if we're going to do a post_update() at all, seems best to do it right here in this issue.

Meanwhile, any release manager thoughts on remaining task #1: allow_authorize_operations setting?

Thanks!
-Derek

Re point #1: allow_authorize_operations setting:

My proposal would be to remove it from the default.settings.php files for new sites, but not formally "deprecate" the setting and generate warnings if sites have used it to opt-out of the Update Manager already. No real harm in leaving it in settings.php files, even if it's being ignored, right?

Maybe that is worth a D12 follow-up to formally deprecate the setting and trigger warnings about still having it defined?

Per Slack discussion with @nicxvan, decided to:

• Add update_post_update_clear_disk_cache() here to clean out the cache already. Then we don't have to mention that in any release notes. Updated the CR accordingly.
• Added a note to 📌 [12.x] Remove all legacy code related to authorize.php and FileTransfer Postponed about a duplicate post_update in D12 once all the code is also gone, just in case.
• Remove allow_authorize_operations from default.settings.php.

I'm reluctant to already deprecate the setting. In theory, if any contrib or custom code is still using any of the deprecated code paths, the allow_authorize_operations killswitch still might be validly preventing weirdness. So we don't want people to remove that from their settings.php files until all the code that it kills is already dead and gone. Wearing my former security team hat, any site that already used $settings['allow_authorize_operations'] = FALSE; gets a gold star for wanting to manage their site code in a better way. I don't want to nag them to remove that killswitch until we're absolutely sure nothing is relying on it.

I think it's OK to do 📌 [pp-1] [12.x] Deprecate the 'allow_authorize_operations' setting Postponed , the point about waiting to bother people who already opted out makes sense.

#27 is a good point, if we don't think anything can populate those directories, might as well remove in 11.2

Great, thanks! Crossed 1 off the remaining tasks.

Last point that needs a decision is remaining task 4:

Decide if update_post_update_clear_disk_cache() needs automated tests.

Then I think a release manager can remove the “ Needs release manager review” tag and this could be RTBC.

Thanks!
-Derek

I don't think we need explicit test coverage for that - we have implicit test coverage in that the update will run during update tests. Removing the RM review tag.

Great, thanks again!

This looks great, I confirmed it deprecates what it claims, all messages look right.
Confirmed the CR outlines all deprecations.
All deprecation messages look correct.

Some of the methods here probably don't really need deprecating, like underscored functions, but also deprecating them means we don't need to find them all again to remove in 12.0.0 and it doesn't do any harm.

Committed/pushed to 11.x, thanks!

catch → closed merge request !11936

Fantastic, thanks!

I published the CR → . I also added a note to the 1st Update Manager deprecation CR → to link to this CR.

Glad this landed so we can cleanly remove all this stuff once 12.x is open. 🎉

Thanks again!
-Derek

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