🇺🇸United States @dww

📌 Use hook_requirements_alter() to only silence the warning, not take over the whole update.module namespace Active now has a working MR for a new branch. See also 📌 Add dww as a co-maintainer for the 2.0.x branch Active . Or, I could move it into a whole new contrib project, something like update_hide_uninstalled_warning or something.

See also 📌 Add dww as a co-maintainer for the 2.0.x branch Active

dww → created an issue.

MR open for a new 2.0.x branch.

Wonder if I should just move this into a whole new contrib project called update_hide_uninstalled_warning or something, instead.

Tagging so we try to fix this before we ship any releases with the old way

Opened 📌 Use hook_requirements_alter() to only silence the warning, not take over the whole update.module namespace Active for the better solution to this problem.

dww → created an issue.

@solideogloria: I think you misunderstand my point and the intention of that contrib module. It (was?) is implemented using a methodology that might have been necessary pre 9.5.0 core, but which has various problems. There's a better way now. If that contrib was ported to modern Drupal and used hook_requirements_alter(), it would actually do what it's supposed to - silence this warning. That's the whole point why that contrib module exists. So if you're in one of the use cases from comment #15 (or any other use case) where silencing this warning is actually a good move, install the (updated, and working properly) contrib module, once it has a new release. 😅

But we don't want to change this behavior in core, for all the reasons the release managers have already written (and which I fully support).

The problem is we don’t want to keep debating this case by case. The proposal is that @final gives us everything we actually want to use final for, with none of the downsides. So we should never use final and always use @final. Then it’s totally clear, we don’t waste time continuing to argue about this in individual issues, and everyone gets what they want. 🙏

Given #309040: Add hook_requirements_alter() → is in core since 9.5.x and up, seems the update_notifications_disable contrib could be ported to use that mechanism to suppress or alter the warning, not the problematic methodology it used per the summary here.

I’d strongly be in favor of closing this as “works as designed”

+1 from me. I can’t imagine wanting a separate class for each form ID you want to alter, and wanting an __invoke() method on each one.

Removing this parameter, at least from FormAlter and Preprocess, would also help minimize the yuckiness of 📌 [pp-2] Clean up method default to use NULL for Hook Attributes Active . 😉

Looks really good, thanks!

Mostly documentation nits, but a few param types which would be good to sort out before we commit and ship this.

Pretty sure we're now down to 2. 🤓

Is there a meta or plan for template preprocess conversions? I haven't been following those. Happy to try to help if I can.

Re: child vs. related, there's no hard/fast rule. But generally, if something is a "follow-up" that'd be a child issue. Especially for meta/plan issues, all the steps in the plan should be child issues. Related is for exactly that: related. It's not directly a "descendent" or "ancestor" of the issue, but it's somehow relevant to also consider for whatever reason(s).

For this plan, removing all the related issues that are already children so they're not listed twice. The remaining two are "siblings" of this issue (other children of the same parent). 🤓 But for better visibility, might as well leave those. 😉

Thanks for the bug report. Oh interesting. I think this changed between group V1 and V2+, and we missed it during 📌 Add compatibility with Group v2 and v3 Fixed .

Opened a bunch of MR threads for some things that could be improved.

Also tagging for some basic automated tests for this new widget. Trying not to commit anything that doesn't have test coverage.

Thanks again!
-Derek

Interesting, thanks for the issue and MR. Not sure there's much precedent for "read-only widgets", since those are usually formatter plugins, not widgets. 😅 But I can see why having this info visible on an entity edit form would be useful.

Pretty simple, other than 📌 Move REQUIREMENT_* constants out of install.inc file Needs work . Pipeline is green on both MySQL and PgSQL.

I keep adding @todo comments to core MRs with @see pointing here. We really need a better solution than manually including install.inc, or worse, using loadInclude($module, 'install') to get it to happen magically. 😂

Oh hah, I forgot that InstallRequirementsInterface::getRequirements() is already public static. 😂

Working on an MR, stay tuned.

Cool, thanks.
More accurate title. 😉

Blocker is in.

p.s. Are we sure this is "Novice" material? This stuff is pretty thorny, with some tricky bits to get right. Not sure we want this to be someone's "first contribution to core", etc...

@larowlan: What's wrong with my plan to just instantiate the install-time object in the runtime OOP method and call it directly? Why does it have to be moved into a static method?

We might have other chances to establish the "state of the art" in this space over at 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active , but this is a much simpler initial case to tackle. 😅 Personally, I think it's cleaner to instantiate the install object and use it at runtime than it is to refactor both to rely on a static method. Whatever DI hurdles / restrictions the install time object must conform to means it's a subset of what would be allowed at runtime, so I don't see how it could break.

Thoughts?

Thanks,
-Derek

Minor edits to the summary to reflect reality, and converting this meta into a plan.

These are now committed to 11.x:
📌 Convert hook requirements that do not interact with install time Active
📌 Convert hook_requirements that do interact with install time that are not system Active

That leaves these as the only blockers to 📌 [pp-many] Deprecate hook_requirements Postponed , right?

📌 Convert experimental_module_requirements_test_requirements to new Class Active (trivial and RTBC)
📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active (massive and NW)

Wonder if it's possible to finish #3493718 in time for 11.2.0-beta, which would allow us to deprecate hook_requirements() still in 11.2.0 for removal in 12. Otherwise, I think that'd be a disruptive enough change that it'd be a 11.3.x deprecation for removal in 13...

Thanks,
-Derek

p.s. Also wonder why we're marking child issues as related. 😉

Giving this its proper parent.

Agree this is RTBC. Small changes. The new class implementation is identical to the old procedural version. The comments look fine. I might split hairs, but it's not worth it. 😂

Thanks!
-Derek

Thanks!

Giving this its proper parent issue for posterity...

Re: #13: per https://www.drupal.org/project/api →

This project is currently under the active maintainership of drumm and fjgarlin.

Let’s be more specific so we don’t confuse ourselves. 😅

Re #3: isn’t that 📌 [pp-1] Review duplicated requirements check in package manager Postponed ?

Sorry, wrong link. See related issues.
📌 [pp-1] Review requirements duplication Postponed

Replied re duplication. Back to RTBC.

Update: both 📌 Rename update module back to Update Status Active and 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active are now done, in time for 11.2.0-alpha1. So there's nothing in the legacy "Update Manager" to worry about for moving this forward.

Thanks!

Sweet, thanks for both!

Mentioned in Slack, but I'll mention here, too...

Do we want to open a related issue about setting up a CI pipeline to run cspell on everything in this repo? Any other lint jobs we can think of?

Thanks! I probably should have credited you for this issue in the first place, since you pointed out the same problem with the initial MR I put up at 📌 Finish deprecating 'Update Manager' related code in Update Status Active . So even though that review normally wouldn't qualify for issue credit, you were the inspiration for me noticing this bug and fixing it. Therefore, saving credit for both of us.

Whoops, 1 more (hopefully quick and easy): 🐛 Fix trigger_error() in legacy Update Manager deprecations to use __FUNCTION__ Active

I created the initial main branch for this repo. Then I created an MR from the issue fork.

dww → made their first commit to this issue’s fork.

dww → created an issue.

With 📌 Finish deprecating 'Update Manager' related code in Update Status Active now in, I'm going call this plan RTBC. Removing release manager and product manager review tags that I added back when this was an MR to directly remove everything. Input from those folks resulted in this becoming the more cautious plan as it now stands.

I think this can now be closed, but I'll let someone else decide.

Thanks!
-Derek

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

Even more clear summary. 😅

Saving credit for everyone.

Pipeline is now green. Trivial change. Summary and title are clear.

I made the same suggestion at 📌 Add order parameter to FormAlter attribute Active so +1 for me. Will RTBC when the pipeline is green.

x-posted a title change, but thankfully d.o prevented that. 😅 Even more accurate title now. 😂

I think this needs a rebase and resolving some merge conflicts after 📌 Finish deprecating 'Update Manager' related code in Update Status Active landed.

This will conflict with 📌 Add order parameter to FormAlter attribute Active which is more urgent. 😉 Let's re-roll this once that lands to focus on the inheritance per the summary and title.

Suggestion applied, pipeline is green again. I think we can sort out the public stuff at 📌 Rethink #[Hook] attribute inheritance Active

Crediting @donquixote for pointing out this omission (per comment #5), and MR and Slack reviews.

Very small nit. Otherwise, this looks ready.

Yeah, the original FormAlter issue was open and in progress before the order issue landed, so we forgot. Also mentioned at 📌 Rethink #[Hook] attribute inheritance Active

Agreed this is RTBC.

Bumping prio, tagging, and adding related

I RTBC’ed the form alter issue, but I agree this is cleaner and better DX. still waking up, so not a thorough enough review to RTBC (yet), but in principle, +1 from me. More later.

Thanks!
-Derek

Sorry to do this, but back to NR based on a new MR thread. I'm probably just confused, but I don't understand some stuff, so I want to retract my RTBC until I'm clear.

I have not completely re-reviewed the entire MR changes tab. But I reviewed the individual commits since the last time I did so and everything seems in order. The pipeline is green again. Back to RTBC.

p.s. Noticed while re-reviewing, but we really need to fix 🐛 Singular variant for plural string must contain @count Needs work and formalize that. core/modules/node/src/Hook/NodeRequirements.php gets this wrong, but so does the original code in node.install.

p.s. I'm unclear why core/lib/Drupal/Core/Updater/Updater.php is being touched, since that is in fact an abstract class, so the existing deprecation is following the documented procedure.

This MR seems wrong.

Whatever changes were done in #6 are all gone.

#13 is also worrisome. "After verifying that the I18nQueryTrait is not used anywhere, I’ve removed it entirely." 😬 That's not how this works. 😅We mark something deprecated and *leave it where it is* so that contrib and custom code isn't immediately broken. That's the point of deprecation. So yes, it's unused in Core. But it needs to remain until D12 when we told people we'd remove it from its current location. I'm afraid of whatever edits were made to a live CR about this. Those probably need to be reverted, too. Although thankfully, https://www.drupal.org/node/3439256/revisions/view/13881188/13973995 → doesn't look so bad (other than using markdown syntax which isn't yet supported on d.o CRs and docs pages).

I'm not sure this issue is really suitable for "novice" contributors.

I don't know of any docs specific to "how to write good code for Core" anywhere. 😅 Here are some of the spots I found while searching for a home:

https://www.drupal.org/community/contributor-guide/skill/php-codingprogr... →
https://www.drupal.org/community/contributor-guide/task/create-a-merge-r... →

Neither of those are particularly discoverable or exactly relevant.

Probably more likely to be found if we considered this a "Coding standard" not a core-only policy, but that's a bit of a slippery slope.

Maybe we want a new child page under https://www.drupal.org/about/core/policies → for stuff like this. Something like "Core coding policies". Would also be somewhere to document the outcome of 📌 [policy, no patch] Hook classes should not be marked final Active vs. 🌱 Use final to define classes that are NOT extension points Active . We could write up 🌱 [policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible Needs review there, too.

With 11.2.0-alpha1 coming very soon, I circled back here. Crossing this off the remaining tasks:

Decide if we can remove lib/Drupal/Core/Archiver/* and related tests, too.

I did some searching and discovered those tarball-manipulating classes are used by core's Config module:

• core/modules/config/src/Controller/ConfigController.php
• core/modules/config/src/Form/ConfigImportForm.php

Nothing else to do there. Those are the least yucky parts of the original/legacy Update Manager 😅 and I don't believe they're the source of any maintenance troubles, so I don't feel bad leaving them in core after everything else is removed. 😂

Also crossing this off:

Decide what to do with the administer software updates permission. It's also used for update.php (DB updates), so I think it needs to stay, but TBD.

In addition to update.php (DB updates), this perm is heavily used by the automatic_updates contrib module. So that permission is here to stay.

Therefore, once 📌 Finish deprecating 'Update Manager' related code in Update Status Active lands, we're basically done with this plan, and everything else will happen at 📌 [12.x] Remove all legacy code related to authorize.php and FileTransfer Postponed .

Groovy, thanks.

Where might one review the list of beta blockers, outstanding stable blockers, etc?

I assume the answer is no, but wanted to ask: I assume the rebranding of auto updates to “Update Manager” per 📌 [policy] Core module machine name and UI labeling for Automatic Updates Active and others doesn’t impact package_manager at all, right? There are no functions that might need renaming, correct?

Thanks,
-Derek

Great, thanks again!

Might as well comment here for posterity, since Slack is ephemeral.

Yes, I hate final. I know we’re all excited about limiting our API surface, and I know the lengths we go jumping through hoops to provide BC in some questionable circumstances. From the standpoint of maintainers, the appeal of final is really juicy. But using a language “feature” to completely prevent extending a class is a real burden in various scenarios. Both because of “interesting” requirements when building real things, and when trying to write tests

I love the @final annotation as a solution. It’s totally clear, unambiguous, it invites documentation, but it still lets you extend if you need to. You know you’re on your own, but you can do it.

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

Indeed. While 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active is done, 🌱 [meta] Deprecate tarballs, because they are incompatible with Composer-managed dependencies, Automatic Updates, Project Browser, and release managers' health Active is not. So I agree there’s still value in locking this down.

More accurate status

Indeed, the legacy “Update Manager” is now deprecated and the UI has been removed.

📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active

Not going to fix this after all. Hopefully the new “Update Manager” (the new name for “auto updates”) handles this better.

Yeah, I don’t think we’re ever going to add this. 😅

Also fixing remaining tasks. All the screens from update.module have either been removed or the names changed. Only possibly follow-ups would be for auto updates.

It was just misfiled 1 header up. 😂

I think this is being worked on for the new 'Package Manager' and 'Update Manager' plumbing. Certainly won't be added to the legacy 'Update Manager'. See 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active . Tempted to close this as 'outdated' or something.

Decided to deprecate all the plumbing and remove the UI in 11.2.x.
See 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active
Closing this as outdated.

Decided to deprecate all the plumbing and remove the UI in 11.2.x. Closing this as outdated.

No longer relevant. This is more about something like Project Browser and being able to use it to browse for recipes during initial site installation.

We've removed this form, so no sense trying to make it more theme-friendly.

Ugh, sorry we never fixed this. Now the UI has been completely removed (starting in 11.2.0). Probably not worth fixing this bug at this point.

The Update Manager has been deprecated or removed from core. This is no longer still relevant to the 11.x branch.

Great, thanks for confirming! NW for AttributeDiscoveryWithAnnotationsAutomatedProviders::parseClass() then.

PHP Fatal error: Uncaught Error: Call to undefined method Drupal\Core\Hook\Attribute\Hook::setMethod() in /builds/issue/drupal-3512835/core/lib/Drupal/Core/Hook/HookCollectorPass.php:289

Initial pass at saving credits:

berdir and duadua for the code
catch, andypost, quietone, godotislate, myself and larowlan for MR reviews

The 1-off phpcs fix from prabha1997 which didn't resolve the problem and was the only effort here at all doesn't seem to qualify. Nor the other commenters so far.

Between #62 and the MR thread nits, bumping back for at least NR.

Don't want to un-RTBC, but while reviewing the MR, I grepped the code for other references to this nid, and found:

modules/migrate/src/Plugin/Discovery/AnnotatedDiscoveryAutomatedProvidersTrait.php

/**
 * Provides method for annotation discovery with multiple providers.
 *
 * @internal
 *   This trait is a temporary solution until annotation discovery is removed.
 *   @see https://www.drupal.org/project/drupal/issues/3265945
 */
trait AnnotatedDiscoveryAutomatedProvidersTrait {

core/modules/migrate/src/Plugin/Discovery/AttributeDiscoveryWithAnnotationsAutomatedProviders.php

    // @todo Handle deprecating definitions discovery via annotations in
    // https://www.drupal.org/project/drupal/issues/3265945.

The first seems like we would ignore that @see until we completely remove annotations, not just trigger deprecations. But I'm unsure on the second. That seems like maybe something else we need to deal with here?

I'll keep looking at the MR, but wanted to post here since these files don't yet show up in the diff and it's not so obvious how to open MR threads about them...

FYI: 📌 Rename update module back to Update Status Active is now done.
📌 Finish deprecating 'Update Manager' related code in Update Status Active is the current priority to finish before 11.2.0.
Then we should be good until we can remove everything and tidy up loose ends at 📌 [12.x] Remove all legacy code related to authorize.php and FileTransfer Postponed .

Thanks!
-Derek

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.

dww → created an issue.

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?

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

FYI, 📌 Rename update module back to Update Status Active is now done, paving the way for this to happen with minimal confusion. Can this policy be marked fixed, now?

Wasn’t on the roadmap, directly, but 📌 Rename update module back to Update Status Active so we can safely rename this issue to match the current naming plan. 😅

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.

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.

Agreed this situation is very troubling. Thanks for opening this issue and considering what to do about it.

Not sure how folks feel about having their Slack transcripts posted publicly. I've got txt versions stashed locally, could upload if desired / needed / agreed.

Personally, I found the "hey, why don't you give me some free mentoring and tell me what's wrong with my code if you don't like it?" attitude very disturbing. People who objected were framed as the villains, squelching an enthusiastic contributor and being unwelcoming as a community. I believe they were rightfully objecting to AI-generated slop being posted, then asking the community to review it in ways the "author" couldn't be bothered (or wasn't able) to do themselves.

Our time is not free. Just because you're posting it as open source code doesn't obligate anyone to "mentor" you. If you want to hire someone to do a security audit on your code, go ahead. But don't post slop garbage as your "contribution", then expect experts in the community to teach you everything the LLM got wrong for free, as if you're entitled to being their apprentice and learning from them solely because you're making the code available under a GPL license.

Link to the CR, not directly to the issue trying to add the new Update Manager.