Account created on 19 January 2006, over 19 years ago
#

Merge Requests

More

Recent comments

๐Ÿ‡บ๐Ÿ‡ธ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++

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

I'm open to improving this. MRs welcome. ๐Ÿ˜‰

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

๐Ÿ“Œ 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"...

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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".

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธ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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

@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).

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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. ๐Ÿ™

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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โ€

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

+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 . ๐Ÿ˜‰

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Pretty sure we're now down to 2. ๐Ÿค“

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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. ๐Ÿ˜‰

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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 .

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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. ๐Ÿ˜‚

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Oh hah, I forgot that InstallRequirementsInterface::getRequirements() is already public static. ๐Ÿ˜‚

Working on an MR, stay tuned.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Cool, thanks.
More accurate title. ๐Ÿ˜‰

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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...

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

@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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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. ๐Ÿ˜‰

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Thanks!

Giving this its proper parent issue for posterity...

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Re: #13: per https://www.drupal.org/project/api โ†’

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Letโ€™s be more specific so we donโ€™t confuse ourselves. ๐Ÿ˜…

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Re #3: isnโ€™t that ๐Ÿ“Œ [pp-1] Review duplicated requirements check in package manager Postponed ?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Sorry, wrong link. See related issues.
๐Ÿ“Œ [pp-1] Review requirements duplication Postponed

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Replied re duplication. Back to RTBC.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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?

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

dww โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Even more clear summary. ๐Ÿ˜…

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Saving credit for everyone.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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. ๐Ÿ˜‚

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Very small nit. Otherwise, this looks ready.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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 .

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

Groovy, thanks.

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

๐Ÿ‡บ๐Ÿ‡ธUnited States dww

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

Production build 0.71.5 2024