🇺🇸United States @dww

I’m still not convinced, but not worth it to keep arguing. I’m happy to stand aside and let the majority view prevail here. 😉 HookDocumentation it is.

I think the only other concerns in here are very minor docs nits that could be fixed any time, so let’s get this in before beta gets tagged and we can’t fix any of it.

RTBC++

Try to look at this with fresh eyes:

#[HookDocumentation('hook_form_alter')]

You call that documentation? 😂

I call it a name.

I think HookName would be way more clear. The fact it's linking to documentation is sort of a side effect. Yes, it's why we're adding it. But it's not documentation. It's a pointer that we use to find the documentation.

Most of the discussion was in a gargantuan Slack thread:
https://drupal.slack.com/archives/C079NQPQUEN/p1746863182915979

I'll agree that "HookDocumentedAt" is a little cumbersome. However, I think "HookDocument" is too ambiguous.

Looking at it with fresh eyes, what about "HookName"? Isn't that what this is really about"? The name of the hook that children of Hook are targeting. The fact we use this name to help find the right documentation is relevant, but it's not necessarily the only use for this.

p.s. Saving credit for the 4 of us who've been in all the Slack threads, MR threads, etc.

FYI: Thanks to this effort, I've been able to close about a century's worth of old issues out of the update.module component. 🎉

https://www.drupal.org/project/issues/search/drupal?status%5B%5D=5&statu... →

Or see the growing list of issues in the "Referenced by" list in here, since I've been marking things related...

Huzzah!
-Derek

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

With 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and all child issues (and 📌 Rename update module back to Update Status Active ) all fixed, this is now separated. We're back to core only providing point #1 from the summary. Calling this plan fixed.

I'm open to improving this. MRs welcome. 😉

The Update Status module in core (formerly known as "Update Manager") did not support updating core via the UI at all. It only handled contrib modules.

I believe the new "Update Manager" from the "Autoupdates Initiative" will log things that it's doing.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Remove adding an extension via a URL Fixed is done, all this code is deprecated and will be removed in D12. The UI is already gone in 11.0 and 10.4.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, all this code is deprecated and will be removed in D12.

📌 Convert update_calculate_project_update_status() into a class Needs work would be a helpful start, without diving into everything mentioned here.

Thanks to 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active 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"...

Well, "autoupdates" still isn't "done", but we moved forward, anyway. Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, this code is all deprecated and will be removed in D12.

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, this code is all deprecated and will be removed in D12.

With 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues complete, we're back to a single "Available updates" report, so closing this as "outdated".

Now that 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and child issues are done, this code is all deprecated and will be removed in D12.

#3205746: Group the available updates report by status, make each collapsible → is another way to improve the same basic problem.

Similar problem, different solution as #3205746: Group the available updates report by status, make each collapsible → ...

https://www.drupal.org/project/update_notifications_disable/releases/2.0... → is now out.

Cool, thanks!

Merged this into a new 2.0.x branch.
Cut a 2.0.x-dev release.
Tagged 2.0.0-rc1 → .

Calling this fixed.

📌 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!