🇺🇸United States @dww

See discussion in ✨ Add Alpha level Experimental Automatic Updates module Needs work . The core thing is going to be called update_manager, not auto_updates.

I believe "Project Browser" is also already set. I haven't seen any interest in trying to rename that.

package_manager seems to already be resolved, and is already in core as an experimental module, and is what both

Tempted to close this as either duplicate or outdated, since I believe all the decisions have already been made.

Rebased after 📌 Create enums for RequirementSeverity and RequirementPhase Active landed. Removed the yucky now that we've got an enum instead of needing to include install.inc.

Re #3/#6. The pipeline in the new MR failed with: 4384 ergebnis.declareStrictTypes (among other things). Ugh. I know how much we love to "get off the island", but IMHO, the 28 line diff to .gitlab-ci.yml is vastly superior for all sorts of reasons.

Per a Slack thread in #core-development, tentatively tagging this as a change that could be quite visible to end users. I'm not attached that it's actually a "highlight", but potentially worth mentioning. I'll leave that decision to the release managers and highlight editor(s). 😅

Thanks,
-Derek

Slick, thanks for working on this!

IMHO, adding 28 lines to .gitlab-ci.yml seems vastly preferable to both a new dev dependency and an even larger PHPStan baseline.

p.p.s. As a parallel example, we have 🌱 [policy, no patch] Use declare(strict_types=1) in new code Active . We already add strict_types to all new code, yet the vast majority of core files do not yet declare strict types. I don't think it's the end of our world if we have a policy (as this one was previously worded) to "use @final, not final", even if there are still a handful of legacy final usages sprinkled around core.

Re: #63: All fair points. Another package_manager contrib was indeed a hypothetical example. 😅 Didn't mean to pour salt on an open wound. 😬 That said, I fail to see how using @final in those cases would make things any worse than they already are. My main point with #63 is a general one that "experimental" code doesn't seem more appropriate for final than stable code, so I don't love the proposal in #60 (which I assume is a genuine attempt to find compromise and to move this proposal forward, not a strongly-held desire to use final at all).

I'm less concerned about un-finalizing the existing usages, other than for consistency. I'm mostly advocating for using @final not final for everything new, and having a clear policy that doesn't involve arguing about 2 competing mechanisms to accomplish the same thing. I care less about phasing out final in the small handful of existing cases (although I don't think it would be difficult if/when we decided to do it).

p.s. I feel your pain about the priorities of the corporate funders. That explains both the AI Initiative and the lack of support for package_manager / update_manager, etc. It's too bad the people who know the system the best don't have more control over where the development $$ (and therefore, effort) flows, and that those decisions are left to the business people who want to chase the shiny and new (and speculatively profitable), not necessarily the important. Welcome to late stage capitalism, where the golden rule is that those with the gold get to rule. I know I'm not saying anything you don't already know (and mostly/entirely) agree with, in this paragraph. 😅 ✊

Solidarity,
-Derek

Re: #59:

So it seems very uncontroversial to me to add final to a load of new places that aren't using it

Typo? Did you mean "to add @final to a load of new places..."?

Re: the idea that final is okay in experimental modules... not saying this really happens much, but isn't the time when new core code is still experimental a great opportunity to try to extend and improve it in contrib? How much experimentation is possible if things are locked down as final? If I want to make an even better package_manager in contrib, my hands are pretty tied by tedbow and phenaproxima's prolific use of final.

Here's an example of an MR to OO-ify one of the most complex functions in all of core: 📌 Convert update_calculate_project_update_status() into a class Needs work . That MR, as currently written, is full of final and private. I haven't yet picked that apart, but I'm strongly opposed. If I wanted to extend core's Update Status in contrib, an MR like that would mean I basically have to copy/paste most of the relevant code. No thanks.

I'm okay with having to debate whether something is meant to be an extension point on a case by case basis. I just don't want to have to debate the mechanism to declare it's not meant to be extended. I want to save all the effort, from both committers, subsys maintainers, and contributors. We waste^H^H^H^H^H spend enough time arguing about things as it is. 😅 I'd love it if we could agree with what I wrote in #33:

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

Indeed, I meant no offense. I only mean that we don’t have a formal “policy” around final, so I didn’t want to really call it that. 😅

But that’s not the main point of #54, nor what I wanted to solicit a response about. Here’s the TL;DR:

As I understand it, the desire is to clearly mark classes as “do not extend”. Either a class is intended to be extended or it’s not.

and

To me, we need a tool to mark classes as “do not extend”. We’ve currently been using final, and there’s a movement to stop doing that and use @final instead. But I can’t yet fathom that we’d have 2 sets of “do not extend” and why we would use different tools to communicate that for the 2 “different” sets.

Yeah, or we start over with a clean issue, free of confusion, scope changes, and tantrums, so there’s a clean comment history to read.

I don’t understand the point of trying to craft a policy or heuristic for when to use @final vs final. As I understand it, the desire is to clearly mark classes as “do not extend”. Either a class is intended to be extended or it’s not.

Some (many?) of us hate final as the way to communicate that, since it locks people out, goes against the spirit of open source, and makes it impossible to do “interesting” things when necessary, including in various scenarios when you want to mock things for testing.

The proposal was to use @final to communicate “do not extend” in all cases, since it still clearly identifies things that are not meant to be extended, invites documentation about why not or what to do instead, but it doesn’t actually prevent it if people want or need to “break glass in case of emergency”…

That’s why the scope expanded in this issue to encompass a complete solution. If we break up the scope into the 3 issues proposed in #44, it doesn’t make sense to me. I don’t see how we can “invent a policy for when @final is okay”, separately from “mark things that aren’t expected to be extended”, which is identical to our current “policy” for using final.

This was envisioned as a policy issue, not an implementation. If we decide that @final is a better tool for the job, we’d open follow up issues to start using it, convert existing final usages, etc.

Would someone who wants to keep using a mix of both final and @final to give some indication of how we could possibly tell the difference from a policy perspective? To me, we need a tool to mark classes as “do not extend”. We’ve currently been using final, and there’s a movement to stop doing that and use @final instead. But I can’t yet fathom that we’d have 2 sets of “do not extend” and why we would use different tools to communicate that for the 2 “different” sets. Please explain. Thanks!

@chx: my brother, you’re not helping these issues with your drama. There are some possibly effective and productive arguments against what xjm has written, but “she killed it because she hates me” is not one of them. You’re poisoning the well for everyone who agrees with you and wants to see this happen. Now, in addition to whatever technical arguments we might make, we’ll have to overcome the (righteous) disdain for your tantrums.

If you get discouraged, disappointed, burned out, frustrated, etc, please just ignore and “walk away”. But your angry and unhinged public comments make it so much harder to get anything done.

authorize.php and the FileTranfser classes are now deprecated and slated for the dumpster.

Hi folks. Thanks for following up. Still not going to add other maintainers, since it’s not “my” module. But I will have some time on Wed and Thu of this week to try again to move some MRs forward and cut a new release.

Probably now obsolete in favor of project browser.

Note: I’m in the middle of an extremely busy time right now, and will be totally offline for a few weeks at the end of June and early July. There’s a tiny chance I’ll be able to do anything with this until mid July. So if anyone else wants to pick this up and run with it before then, please do!

Now that hooks can be implemented with attributes, can’t we have bundle classes implementing any needed hooks directly? What else is there to now explore?

Thanks for opening this to address my MR feedback. Agree that handling it here after the parent lands is fine.

So far, changes mostly look good. Opened 1 MR thread, though.

Also, since cspell is the primary thing we want CI to enforce, and since there are no other “tests” we might run, regardless of spelling errors, shall we set allow_failure: false for that job?

Indeed, see #2:

This might be more trouble than it's worth.

…

dww: Stepping back a bit -- is this even worth it?

…

I opened this issue precisely to discuss the cost / benefit. 😅

I commit this but there isn't the automated message. Why is that?

Because the commit message wasn’t formatted at all, there’s no issue NID (nor credits / attributions) and so there’s no way to know what issue to put the automated message in.

I hate the default commit message provided by the d.o UI, but at least it includes a NID so that the git history points to the issue history. 😅

Might be worth reverting the commit and doing it again with a properly formatted message.

Since there are no releases, I wonder if we could even get a force push allowed here so we could remove the noise from the canonical history (especially so early in the repo’s life)…

The classes in the summary implement the update.manager service.

Also, the constants in UpdateManager are sprinkled throughout the code. Could make an enum out of them, and deprecate the constants.

I tried and failed to reproduce this locally, with both D10.4 and D10.2. That's because 🐛 Error TypeError: Drupal\Core\Field\FieldTypeCategory::getDescription() if a FieldType has 'description' missing in its annotation Fixed is already fixed, so core no longer crashes when this description is missing. But still worth adding something. 😅

That said "The duration field type." is not very helpful as a description. How about:

"Field to store lengths of time with configurable granularity."

?

Is that too much tech jargon for the site builder UI?

We don't tend to "chase HEAD" like this. This is a brand new deprecation in 11.2 (not yet released) to prepare for D12 porting. It's going to be quite a while before we care about this deprecation. 😅

Should this now be “Closed (outdated)”?

Still haven't had time for a thorough look at the meat of this MR, but I keep finding nits on the outskirts. 😂 Won't bother NW'ing over these, to encourage real reviews of the real changes, but wanted to open threads for now. Hopefully I can make some time on Tuesday to do a real review of the important parts.

@chx: really? Please. The drama is helping no one. phpcs is a tool. It’s not hard to use. You can configure your IDE to get it right. You don’t need to take it so personally and get so worked up and angry over it. Please take some deep breaths, and maybe a day off, but spare us the public agony and tantrum over how having standards is an impediment to velocity.

We’ve been in this together for nearly 2 decades now. You know how much respect I have for you and your abilities. Please know I’m only writing this out of love and care. I invite you to relax and have some perspective.

Thanks,
-Derek

First real look at the MR. Mostly nits. A few questions of substance. Out of time for now. More later...

smustgrave → credited dww → .

By request from chx in Slack, adding more credits

Blocker is in. Needs rebase and conflict resolution to use the new enum.

Blocker is in. Rebased.

Not to be a broken record, but I hope "direct-write" means "add to the queue for the cron job that runs as the user that owns your files" in the security-in-depth world where httpd does not have direct write access to your codebase, right? 😅

@drumm: Thoughts on what URL (structure) we should be pinging for these requests?

@all: Anyone more familiar with recipe internals want to run with this? I don't have time to track down all the details and make this work. Please assign yourself and push commits to the MR to remove @todo and make this really work. Thanks!

Probably too late to get this into 11.2.0 at this point, but tagging for now. Maybe a miracle will happen. 😂 If not, we can move it to an 11.3.0 priority...

p.s. Also crediting @drumm and @phenaproxima for Slack support

Preliminary draft MR now up to get this started. Not sure if this should stay under 'recipe system' or move to 'update.module', but leaving it in recipe system for now.

Currently it only creates / deletes a queue in install/post_update/uninstall, and adds an event subscriber to begin to populate the queue as recipes are applied.

Needs lots of help to get the details right in there, and something to process the queue and actually phone home.

Note: thanks to 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active and 📌 Remove adding an extension via a URL Fixed we're back to update.module only being "Update Status", providing the "Available updates" report at admin/reports/updates, email notifications, and the telemetry aspect (basically as a side effect of querying for new available releases).

Note, this would be moving in the opposite direction of ✨ Moving project telemetry reporting from the update module into core Active . I'm not totally opposed to adding this, but wanted to point out that some folks already don't like how the telemetry aspect is bundled into Update Status...

📌 Copy block configuration from admin theme when enabling an admin theme Active is at least related, if not duplicate.

Huzzah!

This will conflict with 📌 Create enums for RequirementSeverity and RequirementPhase Active which is more urgent to get into 11.2.x, so marking this postponed on that one.

Fleshing out API changes section of summary.

Pushed commits to resolve most of the threads I opened.
Created 📌 [pp-1] Remove loadInclude and @todo from Update Status after #3410938 is in Active for 2 of them.
Left 3 still open for feedback from @kim.pepper or other reviewers.
Pipeline is green again. Back to NR.

Thanks!
-Derek

dww → created an issue.

Updated the CR about StatusReport ::getSeverities():
https://www.drupal.org/node/3410939/revisions/view/14001271/14002566 →

Overall, this is a big improvement!

As promised, opened a bunch of MR threads. Many are nits, but I'm concerned we're linking to the wrong CR in some places, not convinced about the namespace for the enum class, and a few other loose ends.

Per MR review, tagging for CR updates to mention `StatusReport ::getSeverities()` is going away.

Initial pass at saving credit. So far, looks like everyone has meaningfully contributed. 🎉

This mostly looks great, thanks so much!

FYI: working on an MR review. It's a big diff, so it's taking a while. 😅 I found some stuff worth fixing before commit. Wanted to update status now to get this out of the RTBC queue. Stay tuned...

Moving to the right component and tagging to be smashed. Haven't tried to reproduce this or confirm if it's a valid bug.

p.s. It looks like only update_system_schema_requirements() includes the untranslated UI text. There are other usages of t() in this file (e.g. in _update_fix_missing_schema()).

Triaging the update.module component. This issue is about core/includes/update.inc, which the @file comment says is "Drupal database update API." Moving to a more appropriate component.

Also, tagging as a bug to be smashed.

Confirmed that this file still includes untranslated UI text. I'm not immediately sure if when these methods are running if "enough" Drupal is bootstrapped to be able to negotiate an appropriate language and translate anything. I suspect not.

Perhaps the better task here is to "Add comments to update.inc to explain why t() is not used in early requirements checks" or something. 😅

Side note: The "update.module" component is for the Update Status module. The update.php script is part of the "database update system". However, since this ticket is also talking about install.php, moving to "base system" as a more appropriate spot.

That said, I'm -1 to this being a "bug" that needs fixing. As has been repeatedly explained by release managers, the error messages on these pages are not disclosing any information that's not trivial for attackers to already know - that the site is a Drupal installation at a given path and that anonymous users don't have permission to access the administrative paths. There's no additional information being disclosed. Furthermore, most attacks are using automated scanning to directly attempt numerous exploits, mostly for vulnerabilities in entirely other CMS platforms. My production site logs are regularly full of requests for wp-admin paths, etc, used only by WordPress. Security by obscurity is not security at all. There's nothing to gain from the security perspective, and a lot to lose from the usability one, to try to further obfuscate these pages.

+1 to closing this with "works as designed".

More specific title.

FYI: this whole test is going to be removed in D12 since it’s testing deprecated code. See 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active

That said, +1 to cleaning this up, if only since the work is already done. 😅

Initial title and summary pivot. Haven’t touched the MR

Thanks for all the updates @nicxvan! I did a pass on all 4 related CRs and did a round of additional edits. They're all published and good to go. Removing the tag and calling this Fixed.

Yay,
-Derek

Bummer, lots of random fails, but it seems that core/modules/views_ui/tests/src/FunctionalJavascript/PreviewTest.php is failing, and that's suspicious. My local can't run JS tests against 11.x right now, so I'm a bit stuck on driving this home right now.

dww → changed the visibility of the branch 2904921-exposed-views-filters to hidden.

Running into this again.

I still don't understand #19. Where's the redirect needed in #16? Views is creating a response with a pager full of links. Each of those links could be "scrubbed" to only include query arguments for filters (or sorts, or whatever) that are set to their default values. These links would all be totally valid. I do this manually all the time and it works just fine. No redirects needed.

Well, fun. core/modules/views/tests/src/Unit/ViewExecutableTest.php is failing. Locally, too. I'll see what I can do about that.

I ran into exactly this bug on a client project.

I've got a view with 2 contextual filters (arguments).
The path is node/%node/reports/sessions.
The 2nd argument is another node, at the end of the URL (so I don't need to put the %node in the path).
If the 2nd argument is empty, I want a summary across all sessions.
The links on that summary correctly link to node/%node/reports/sessions/NID and you see everything for a given session.

So far, so good.

However, when I added some exposed filters to the view, things go wrong. They work fine on the final pages (with both arguments defined), but on the summary page, once I select a filter value and click 'Apply', I'm sent to:

node/%node/reports/sessions/all?type=whatever

The all really breaks things.

So I removed 'all' as a valid exception value in both contextual filter configurations. Then the exposed filter on the summary page uses this as the URL:

node/%node/reports/sessions/%2A?type=whatever

which is just as broken. 😅

I opened an MR to basically implement patch #16 against 11.x core. Works locally exactly how I want. Now, when I filter the summary, I stay on the summary, and I see the reduced totals for each session once I filter by whatever.

No idea if we have enough test coverage to know if this is going to break something else. But let's see what the bot thinks. 🤞

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

The new CR here at https://www.drupal.org/node/3524585 → mostly looked great, but I did some minor edits and enhancements.

However, tagging for "Needs change record updates" to capture this task from #65:

We will need to update these too:
https://www.drupal.org/node/3499495 →
https://www.drupal.org/node/3499788 →
https://www.drupal.org/node/3496491 →

For now, I edited them to indicate they were added in 11.2.0-alpha1 (which I know we don't normally do, but this is a special case).

Bit of a facepalm about how much effort went into this, only to collectively realize we were on the wrong track all along. 😬

But kudos to @nicxvan for looking critically enough to see a more simple way forward.

https://git.drupalcode.org/project/drupal/-/merge_requests/12128 is certainly more clean. The diffstat is way better. 😉 It's mostly removing stuff we realized we no longer want (the FormAlter and Process attributes), and converting a handful of usages back to just #[Hook].

Everything should be as simple as possible, but no simpler.

-- Einstein.

This is definitely simpler for everyone: for developers, for core maintainers, for contrib maintainers, for api.module, for documentation.

Given the MR is removing a bunch of things we don't want, and only adding back raw #[Hook] attributes, I spot nothing to complain about with the diff.

Given the pipeline is green, I know the linting and tests are all happy.

Therefore, RTBC.

Agreed this is urgent before beta1 is tagged, so let's get this in ASAP. Critical++

Thanks!
-Derek

Thanks for this!

Re: 2.1 wins, here's the message I x-posted:

Thanks to 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active I've been able to close about a century's worth of old issues out of the update.module component. 🎉 Many of them bugs (that I tagged for smashing). See the first page or so of https://www.drupal.org/project/issues/search/drupal?status%5B%5D=5&statu... → , or the growing list of "Referenced by" issues on 3491731. Progress!

griffynh → credited dww → .

Re: #56: #[HookInvolved] seems even more opaque, vague, and unclear.

@all: Please, we've got 3 core committers (@larowlan, @catch and @quietone) and 3 prolific contributors (@chx, @donquixote and @nicxvan) in agreement that HookDocumentation is our best choice. Let's leave the bikeshed a nice pale green and move on, okay? 😂

Also, FWIW, e.g. we only have FormAlter($form_id) and if you pass NULL you get the generic 'all' approach, and if you pass a value, you get the specific one. So I'd say for consistency we should do the same with these entity hooks and if you pass an $entity_type value, you get something specific, and if you leave it NULL, you get all types.

Therefore, +1 for a single class that takes an optional arg, not 2 separate attributes.

FWIW, we did not use HookFormAlter or HookPreprocess as the child attribute names, but just FormAlter and Preprocess. So yeah, if they're in the Hook namespace, -1 to also prefixing with Hook.

That said, +1 to documenting the hook and parameters in a real function, not in these hook attribute classes.

Great, thanks for reviewing.

@andypost: re #54: Because this isn't necessarily tied to the "help topic" world, there aren't necessarily help topics about each of these custom hook attributes, etc. We've already been around the bikeshed many times on this name. 😉 I don't love HookDocumentation, but that's definitely more accurate and future-proof than HelpTopic would be.

Good idea, thanks for pointing it out.

How's this?

https://www.drupal.org/node/184005/revisions/view/12764872/13994183 →

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.