🇺🇸United States @dww

I am drawn to the benefits of handling install separately. You make a good case that it’s unfortunate to complicate DI for the runtime / update checks when in many cases those don’t care about install checks.

So even though it’s two independent systems, it might be better DX overall to have the simplicity of the hook for runtime and update cases…

I understand about the class name with the hook approach. I’m okay with putting the right Hook attribute on separate methods for each phase in the same ModuleRequirements class, even if that’s in the Hook namespace and not directly shared with the Install requirements.

I don’t mind 2 hooks (approach 1 over 2). Makes it easy to do whatever you need, and easy to share between them if we default to ModuleRequirements and methods per phase. I don’t think there’s much benefit in approach 2. Seems 1 or 3 would be preferable to 2.

I reserve the right to change my mind if Alex (or anyone) comes up with a better option 4. 😂

Thanks,
-Derek

Re #31 : Indeed, see #12 where 📌 Investigate ModuleHandler::add Active was added as related. 😉

Cool, thanks @catch.

Would a release manager be willing to:

a) Respond to comment #19 😅 - Can we really remove stuff in 11.2 that we deprecated in 11.1 for removal in 12?
b) Make a formal decision on what's happening here.
c) Remove the 'Needs release manager review' tag.

?

Also, do we need product manager review? If so, is that effectively Gabor or Lauri at this point? Should I try to ping them via Slack?

Thanks!
-Derek

I had concerns at 📌 Create hook_runtime_requirements Active but it seems more appropriate to raise them here. As usual, I find @alexpott's views compelling, and perhaps a more clear articulation of what I was worried about.

Generally, I didn't love seeing class FileRuntimeRequirements... seems we want to move to class FileRequirements, and have separate methods for each phase. We should be making it easier and cleaner to share code between the phases. Probably more requirements checks should be happening at multiple phases than is currently the case.

I'm not sure how common it is to need services in these checks.

Can we say that if your Requirements class needs services for update and runtime, which it can't use during install, that you have to have a service getter/setter pair, and that getContainer() returns something valid for during those phases and NULL during install? Tests can call the setters with mocked services. The real code will use getContainer() or whatever.

But yeah, I think it'd be better DX overall to move from a single hook_requirements($phase) to a single new requirements API, not 2 new hooks and a 3rd thing implemented some completely other way.

If install requirements are such a special case, and it really does make sense to handle it separately, at least I think we should unify the hook-based parts to encourage more sharing.

It's still 61 files +788 −683. Can/should we split this up further to be easier to review? Maybe media* and system their own issues, everything else in here? I think that'd get us closer to +/- 500 here.

Glad to see the rest of this committed. Thanks for opening this as a new issue.

I’m open to splitting this further if y’all prefer. It’s big now. 😅

Resolved the thread about the module name typo. Good spot.

Thanks,
-Derek

Meanwhile, amazing work, @bbrala! Thanks so much for picking this up and running with it. 🙏 Our long nightmare of "Issue #xyz by incomprehensible, list, of, user, names, before, you, know, what, changed" is looking to soon be over! 🎉

I'm not a fan of "local tooling should solve this" as an approach. There are 1000s of maintainers on *.d.o. Telling each of them to solve this common need themselves is infeasible. Our collaboration platform handles (nearly) everything else, it should solve this problem in a general way, too.

There's no way to fully automate the credit assessment. That boils down to humans having to make decisions. So it can't all happen "automatically", anyway. If a human has to go through the issue and/or MR to decide who meaningfully contributed and twiddle a UI to record all that, that same UI can both save those results into the DB, and spit out this copy/paste block of formatted text to use for the commit message.

*.d.o definitely can and should have access to email addresses. I'd be in favor of a d.o-stored field attached to user accounts that indicates the preferred email address for commits. That looks like what ✨ Allow users to set GitLab Commit email Active is about. If not explicitly set otherwise, would default to a no-reply. But if users (like me) want to have a valid email address that we control in all our commits (and commit references), we could opt-in to something else. Then this credit UI can honor the user's preference for which email to use for Reviewed-by, Co-authored-by, etc.

Or I guess to make @smustgrave happy, NW is better. 😂 Sorry to change the status to that without explanation, but I’ll open MR threads later with details.

Thanks for this! Exciting.

I’m only looking at the MR on my phone. I have some concerns. I’ll post a thorough review later when I’m not thumb-typing.😅 But I don’t believe this is totally ready in its current state. No rush, since this isn’t happening for 11.1.x. But getting out of the RTBC queue for now so committers don’t waste time on it.

@berdir: I warmly welcome you (or anyone else) to start sketching out a plan at 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active . I doubt we actually want to open an MR there for a bit (hence [pp-3]) but we could already start fleshing out the summary now to do what you're talking about in #32. I agree that'd be helpful work to verify the API being proposed here makes sense, which is why I already opened it, even with my concern about opening sub-issues of 📌 [pp-3] Convert hook_requirements to new classes or hook_runtime_requirements Active for every core module.

I have a scope question at #3490845-4: [pp-3] Convert hook_requirements to new class or hook_update_requirements or hook_runtime_requirements → on how to best split that up. However, definitely system_requirements() will be a huge effort, so I already opened 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active for it and added to #3490845. I'm actually kind of excited about that one. 😅 Maybe I'll be inspired to find time to take an initial stab at it.

Opened 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active to track it. Adding to summary.

dww → created an issue.

For scope, wondering if we really need separate issues for all of these. E.g. the one for update (status) will be a simple move to an OOP hook_runtime_requirements(). Maybe we should group by all modules that only implement a single phase in their hook_requirements()?

• Only needs hook_runtime_requirements()
• Only needs hook_update_requirements()
• Only needs the hook_install_requirements() class
• Separate issues for each module that implements more than 1 phase

I can't wait to see system_requirements() refactored. That's going to help so much. I might even be inspired to raise my hand to at least start on that one. ;)

Thanks for testing! Alas that it's getting harder and harder to resolve this via GitLab.

We're going to have a credit UI on new.drupal.org going forward. I wonder if it's worth adding something to that to spit out an appropriate commit message, like we have on d.o issues now? I wonder how difficult / easy that would be. Maybe easier than trying to implement it in GitLab?

Whereas:

1. All feedback addressed.
2. Pipeline 367282 is green.
3. This is my first time experimenting with requesting changes. Even though I've got somewhat elevated perms in GitLab MRs for core as a subsystem maintainer, I can't actually resolve that request (even though I made it myself). Good to know, I won't be using that feature anymore. The committer can ignore the "Merge blocked: 1 check failed" error on the MR.
4. Tweaked some formatting in the summary.
5. Initial pass at saving credit.
6. I don't mind RTBC'ing this, even though I pushed a trivial commit to update the formatting of the deprecation message.

Therefore, RTBC!

Thanks again,
-Derek

As usual, I believe @berdir speaks the truth. However, this need:

To clarify that, it's not the deprecation that's important, it's when it is safe to use the replacement

is not addressed by putting the exact version the deprecation was added in the messages. You need to read the CR and understand the change to know when the replacement was available, as you explain. But yes, in most cases, it is the same version. So yeah, it's hard to know what's best.

I personally tend to maintain my projects to be as compatible with legacy versions as possible. Sometimes it's impractical and we need a clean break, but often I've found it's barely any technical debt to be able to keep working with very old versions of core. I read the CR, figure out when the replacement was added, add some custom code to call the right thing via `version_compare()` and move on.

I'd be happy either way. I see the benefit of knowing the specific version the deprecation was introduced. I'd be glad to not have to push new commits to open MRs all the time to keep them valid.

Either way, it generally seems pointless to say "is removed in 12.0.0" per #19. So if we keep minor version in there, I'd vote for the middle of the options in #23:

@deprecated in drupal:11.1.0 and is removed from drupal:12. %extra-info%.
@see %cr-link%

Fix formatting bugs with deprecation messages involving __METHOD__ and __FUNCTION__ to always use `() is ...`

The functional test fails in https://git.drupalcode.org/issue/drupal-3481778/-/pipelines/367140 are random, but the fail in Unit is legit. Opened MR threads with suggestions.

Almost there, thanks!
-Derek

The CR looks much better now, thanks! Back to RTBC.

Nice! Love it. Not sure how I missed this issue, but glad to see it mentioned in Slack just now.

All my feedback is addressed. I opened 📌 Deprecate the $use_info_parser parameter to ExtensionParser::scanDirectory() Active for the followup proposed in comments 14-16.

Meanwhile, first pass at saving credits:
@alexpott for all the code.
@berdir for the issue write up and various discussions that lead to this.
@smustgrave, @daffie, @catch for reviews.
myself for reviews, metadata cleanup and opening the follow-up so this can be closed and forgotten without loose ends.

Doesn't seem like the comment from @nicxvan qualifies.

Back to RTBC.

Thanks!
-Derek

dww → created an issue.

Oh, does this need a CR? I guess that'd be the only other possible thing to do here.

I read the MR changes closely, and the issue summary and comments.

@kristiaanvandeneynde makes a compelling case that RenderCacheTest is really broken. There were already a couple of @todo comments in there about removing it. 😅

The actual change to stop the special case context for UID 1 makes sense to me. grepping core confirms that this and core/tests/Drupal/Tests/Core/Session/SuperUserAccessPolicyTest.php are the only places that mention is-super-user. In that test, it's only used to label cases inside a data provider array, not for any actual code, assertions, etc.

I renamed the MR to be self-documenting, but otherwise, all looked great.

I don't see anything left to do here but commit it. RTBC++

Thanks!
-Derek

Thanks! Resolved 2 of the open threads.

For the last one, yeah, https://www.drupal.org/node/3490771 → needs some help:

1. This is no longer true:
   

   Warning: If you later add procedural hooks and need them to be scanned, you must delete this container parameter.
   Setting hooks_converted to false will still prevent scanning, you must DELETE the parameter to re-enable scanning.

2. We should add something about the special non-hook hooks like hook_install() and friends. I don't actually know the gory details enough to fix it myself, but would be happy to review for clarity once done.
3. The instructions for both "No procedural hooks converted to OOP hooks" and "The module still has procedural hooks" are identical, duplicate, and therefore confusing. Both cases are the same, and from the perspective of a developer reading the CR, the difference is irrelevant. The bottom line is there are still procedural hooks, regardless of if some of them have been moved or not. I think we should merge these two sections.

Once those edits are done, this is RTBC to me.

Meanwhile, saving credit for @berdir and myself for detailed reviews, and the aforementioned epic Slack thread. 😂

Opened a bunch of mostly trivial nits / suggestions. A few points of substance.

I nearly reviewed every line. I'll confess I started to glaze over and didn't completely verify that in the cases where we're moving functions around in .install or .module files that the new code is identical to the old.

Resolved all the open MR threads. Then closely reviewed the changes. Opened 3 new suggestions, 2 for pedantic nits, one with a real question about the `$use_file_cache` parameter to the constructor.

After reading the MR diff, here's a first stab at a better title and summary. Further refinements welcome.

Haven't looked at the code. But we can't be ready to commit this if the title and the summary are "Do X or Y". Which is it? 😅

Makes sense, thanks.

Do we want to remove the routes and deprecate all the underlying functions and classes in 11.2.x, then completely remove it all in 12.0.x? Or are you thinking we directly remove everything in 11.2.x?

Thanks again,
-Derek

At #3463226-25: Use the new equivalent updates API to prevent updates from 10.4.0 to 11.0.0 → , @catch raised the possibility we might want another pair of equivalent updates to prevent downgrading from 10.5.x to 11.1.x. Should we open another follow-up for that under this meta?

Thanks for working on this!
GitLab thinks this needs a rebase due to conflicts.
Meanwhile, I opened some MR threads, mostly with pedantic nits.

I bet a lot (though not all) of the trouble from update.module will go away once 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active lands. 😉

Blocker is in

We never remove API changes in a minor or patch release, right? Should it just say:

Deprecated in drupal:11 and removed in drupal:12

?

https://git.drupalcode.org/project/drupal/-/merge_requests/6290#note_423326 is about an actual (minor) bug. NW for that. I did open a bunch of other threads, mostly with suggestions, none of them blocking.

Thanks for opening this! Very helpful to be able to collect all these efforts under a single parent issue.

I'm following the protocols that we use for the coding standards committee (of which I am a member). Even though this is indeed a decided policy, we're not supposed to mark this 'fixed' until all the follow-ups exist.

I don't have time for a deep-dive on this, so for now, doing the quick/easy thing of opening an issue in coder:
📌 Enforce UpperCamelCase case values in PHP enum definitions Active
We can decide there if that's really the best way to enforce this, and if not, move the issue to the core queue and rescope it to enable some existing sniff in the wild or whatever.

Cheers,
-Derek

dww → created an issue.

Saving credit per #27

FYI: https://www.drupal.org/project/pathologic/releases/2.0.0-alpha3 → is now out, with the only change being D11 compatibility.

https://www.drupal.org/project/pathologic/releases/2.0.0-alpha3 →

For clarity, the “cron-driven sudo” step that’s already supported is not via Drupal’s hook cron, which “might be triggered by any user”. I/we are talking about good ole *nix crontab, and specifically setting up the script completely outside of Drupal.

Point 2 in remaining tasks is already done. Adding links.

If you intend to rely on the FileTranfser classes to do this, and you actually think that could still work, you need to speak up (quickly?) in 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active . The current MR there completely removes that entire API (along with authorize.php and related methods from system.module). Everyone (so far) thinks it’s legacy cruft that no one could possibly care about anymore and doesn’t even deserve to be deprecated before removal. 😅

I’m extremely familiar with the goal of UI-based updates for sites configured in depth. I have championed that approach since D6, and wrote the bulk of this code in the first place. You don’t have to explain to me how it works or why in principle it’s a good idea.

However, I’m no longer convinced that any site with httpd running as another user than the one who owns the files will be maintained by someone who is CLI averse. It’s only wild speculation, but my sense is the intersection of those two sets is vanishingly small, and not worth bogging down core with technical debt to support.

It’s absolutely worth making the UI play nicely with a cron-driven “sudo” step. That’s already been done. But actually requiring manually logging in via FileTransfer directly as part of the UI seems unnecessary.

Ugh, sorry. Got busy with too many other things. If anyone else wants to run with this, please do. I'll reassign if I pick it back up.

Closing in favor of 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active . I'd rather convert hook_updater_info() to /dev/null than the plugin system. 😂

Whoops, no need for this to be postponed anymore.

Whoops, this completely fell off my radar. The blocker landed long ago.

+1 for this. Bumping to at least NR. It's only a plan, so the only thing to review is the summary. Seems like universal support for dropping the minor branch version. Should this really be RTBC? What remains to decide this?

Thanks!
-Derek

Right, something like that. Although I don't love configuration knobs of any sort that start with negatives. E.g. no_procedural_hooks: true is kinda weird. "Check this box to turn off the thing" is a UI anti-pattern. But setting has_procedural_hooks: false after conversion is weird, too, since if the parameter isn't defined, defaulting to TRUE is also maybe unexpected. I dunno, naming is hard. 😅

Wow, this moved fast. 😅 nice work, y’all!

One complaint / concern. “Hooks_converted” is sort of vague and potentially confusing. Converted to what? Is it too late to propose something like only_object_hooks or something? The 3 of y’all that worked on this issue are totally familiar with what hook conversion this is about, but I wonder about the other 99.99% of Drupal developers.

Thanks again!
-Derek

Gonna bump this to major, since it's got big implications for auto_updates / new update_manager, Drupal CMS, and since we should strongly consider doing this before 11.1.0 final.

Making the title match the MR. Product and/or release managers can change it back if they disagree.

Indeed, see #3346707-18: Add Alpha level Experimental Package Manager module → and later. Thanks to my raising this red flag 1.5 years ago, package_manager doesn't assume it has write access to your filesystem. 🎉 It supposedly works in both server configurations (just like this legacy 'update manager' stuff from D7). If httpd doesn't have write access, you're supposed to run a CLI command via crontab as the correct UID. See 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed and 📌 Add Symfony Console command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed for details. So it's no longer entirely within the UI. But anyone who is setting up a Drupal site where the files are owned by a different user than httpd (yay for them/us!) is presumably comfortable enough with the CLI to setup such a server-side cron job.

That said, there's going to be nothing for you to "fix" with this legacy UI. 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active is currently completely moving ~5K lines of code. I have no interest in maintaining it as a broken/abandoned contrib module. That train has left the station. 😅

Hopefully, before the new "update_manager" (the new name for "auto_updates" 🙃) lands in core, there will be an option to completely disable unattended updates. Then it truly will be a drop-in replacement for all this cruft, only it'll be speaking composer (and all your external dependencies) instead of manually downloading drupal.org tarballs.

Restoring status. This is a dead end. Please don't waste your time trying to revive it.

Thanks,
-Derek

Added #19 to remaining tasks.

Heh, whoops. Looking a bit more closely at everything I deleted, it includes some of these, e.g. in the Updater* world:

  public function postInstall() {
    @trigger_error(__METHOD__ . '() is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. There is no replacement. See https://www.drupal.org/node/3461934', E_USER_DEPRECATED);
  }

Should we:

1. Scale back the scope of the removals here?
2. Make sure this lands in 11.1.0 and update the https://www.drupal.org/node/3461934 → CR to match? 😉
3. Other?

Thanks,
-Derek

Yeah, core/lib/Drupal/Core/File/file.api.php was already fixed. Pushed a commit for 3 of the others, no question.

I'm not totally sure we should be removing the stuff from stable9. Maybe we need a frontend framework manager to signoff on that (commit b07e3fd3)?

Whoops, hadn't seen #15 when I wrote #16. Some of those are already gone with additional commits I've pushed. I'll cleanup the others now...

Finally got a green pipeline. 😅 Diffstat is now up to: 56 files +11 −5618. 🎉

I'm 80% sure there are still some lingering bits of cruft sprinkled about. I need to do a more thorough job of grepping for various things. But those could be cleaned up in follow-ups if we miss them on this first attempt.

Adding some open questions to remaining tasks:

• 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.
• Decide if we can remove lib/Drupal/Core/Archiver/* and related tests, too.

Fleshed out UI and API changes sections.

This is among the most satisfying MRs I've opened against core in a very long time. 😉 This is maybe too bold. I'm immediately removing everything. No deprecations, just gone. 😂 Current diffstat:

47 files +2 −4963

If we want to mark all this crap (the vast majority of which I originally wrote, so I feel okay describing it that way - no offense to anyone) deprecated and only remove it in 12.x, we'll need a whole new MR. But I wanted to see what happened with this approach for now...

It seems 📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active is the path forward, not this approach. Closing this one.

Thanks,
-Derek

📌 Deprecate/remove the ability to update a module from a URL and authorize.php Active is now the correct dedicated issue for deprecating / removing the authorize.php code and the UI from update.module.

More accurate title, since authorize.php and the UI from update.module don't use URLs -- that was the install new code stuff that's already gone. Also, update.module handles modules and themes, fwiw.

Hah, not sure how I never saw #2352637: Remove the UI for installing/updating modules from update module if it is not fixed in time for release → until now. But that was right in the era when I was AWOL from Drupal and update.module was considered "unmaintained".

Anyway, yes, we should finally remove all this code. For reference/context, in 📌 Remove adding an extension via a URL Fixed "we" decided to directly remove the routes and code for install-new-stuff, instead of doing a deprecation dance and moving the old plumbing into an abandoned contrib module. I personally vote we do the same here.

It'd be lovely to know for certain we'd be replacing this with the shiny/new functionality from package_manager and update_manager/auto_updates in the same release. But even if we don't know for certain, the chances that the legacy "update manager" provided by update.module are actually still helping sites stay up to date with contrib releases are pretty small. More and more of contrib is depending on composer to manage external dependencies, which this code doesn't handle. So unless you've got just the right combo of installed modules, this feature is as likely to break your site as it is to help keep it running the most secure versions of things.

😂

Thanks for all the effort that already went into this strategy and document, and for the ongoing efforts to focus and clarify goals. This is all extremely valuable work.

Perhaps more relevant for Drupal CMS / Starshot, but since it's called out in this strategy document, too, raising this here:

How will "automatic" updates work with contrib, where there's a huge range of quality, rigor, release management practices, funding vs volunteer effort, etc?

Updates for core is one thing. We've got amazing release managers, a ton of process and best practice, and a whole community of contributors and contrib maintainers helping to make sure releases are solid, upgrade paths work, BC is preserved as appropriate, etc. Most of contrib is maintained as a "one person show", often with no funding at all. Some folks intentionally don't follow semantic versioning semantics because they don't want to be bothered with new branches for new features. Tons of contrib modules have little to no automated test coverage. Letting auto_updates (aka the new "Update Manager") apply releases from all of contrib seems like a disaster waiting to happen.

People have been saying for years that "Drupal should be as easy to upgrade as my iOS device", and I keep responding "that ease is only possible since Apple employs an army of developers and others to ensure that all works flawlessly." Drupal core is sort of equivalent, but contrib is most certainly not. If one of the strategic goals for "how we win" is to "lower the cost of ownership" and put an emphasis on "ease of upgrades", what's the strategic plan to deal with the "wild west" of contrib?

Thanks,
-Derek

p.s. Thanks @Berdir for 🌱 [policy] Impact on contributed projects and its maintainers Active . That's related to my concern here, although that issue is about the impact on contrib maintainers if their module is included in Drupal CMS, while my concern is about the impact of contrib maintainers (and their wide variability on release management practice) on end users (both of core/framework + CMS/product).

See #3483501-5: Rename update module back to Update Status → where I asked similar.

Via Slack, @alexpott confirmed:
“ I think adding another setting is a good idea - twig_sandbox_allowed_class_methods seems best”
“ It is a bit least worst possible option - but sometimes that’s what you need.”

I’ll work on a version of this later today.

📌 Harden TwigSandbox methods Needs work is nearly ready. We should postpone this issue on getting that committed, first.

Saving credits. So far, everyone has meaningfully contributed here.

Removing the release note tag thanks to #41.

However, I opened a new MR thread that should be addressed before merging (either renaming the function, or at least fixing the docs, or possibly refactoring this code again).

Indeed, intentional since we need to decide where to follow up to enforce this part of the standard.

Thanks for cleaning this up and moving it along!

I just RTBC'ed 📌 Enable SlevomatCodingStandard.Classes.BackedEnumTypeSpacing Active . I know we're still discussing 📌 Comment style for Enum Active . However, neither of the proposed sniffs seems to check for the main thing we debated here: using UpperCamelCase for the case names. I'm not sure the best way to enforce that. Seems we need another followup, somewhere, but I don't know where. 😅

Bot is happy. MR is simple and does the trivial change requested by the clear issue summary.

I tried some manual local testing to make sure the sniff does what we want. I made the following change:

diff --git a/core/lib/Drupal/Core/Database/Event/StatementEvent.php b/core/lib/Drupal/Core/Database/Event/StatementEvent.php
index 452c200..1033945 100644
--- a/core/lib/Drupal/Core/Database/Event/StatementEvent.php
+++ b/core/lib/Drupal/Core/Database/Event/StatementEvent.php
@@ -7,7 +7,7 @@
 /**
  * Enumeration of the statement related database events.
  */
-enum StatementEvent: string {
+enum StatementEvent : string {

   case ExecutionStart = StatementExecutionStartEvent::class;
   case ExecutionEnd = StatementExecutionEndEvent::class;

Then I did this:

composer run phpcs core/lib/Drupal/Core/Database/Event/StatementEvent.php

Without this MR change applied, phpcs passed. Once I added the MR diff, I got:

----------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 10 | ERROR | [x] There must be no whitespace before colon.
----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------

Then I tried phpcbf to make sure that worked, too:

PHPCBF RESULT SUMMARY
----------------------------------------------------------------------------------------------------
FILE                                                                                FIXED  REMAINING
----------------------------------------------------------------------------------------------------
.../core/lib/Drupal/Core/Database/Event/StatementEvent.php  1      0
----------------------------------------------------------------------------------------------------
A TOTAL OF 1 ERROR WERE FIXED IN 1 FILE
----------------------------------------------------------------------------------------------------

So all is working as expected, and enforcing the standard we adopted.

Nothing else to do here. RTBC.

Thanks,
-Derek

Bot is indeed happy. That commit seems like such a minor and obvious change to the MR, I'm going to be bold and go ahead and RTBC this. If the committer is unhappy about it, we can presumably get a quick re-RTBC from someone else. 😅

Thanks!
-Derek

Looks like random fails in the pipeline. I just retried the jobs. Let's see if the bot is happy with the re-runs (should be).

2 days during a holiday week isn't much time to give new contributors a chance to find and work on it. This is not a release-blocking, urgent task, so there's no need to rush. 😅

That said, since the work is already done, and there's a reviewable MR, leaving the status 'Active' does nothing to help new users, and might potentially lead to confusion.

My local env isn't setup for 11.x tests right now, but I think I fixed it. Let's see what the bot says...

This seems great. Summary and comments all make sense. MR looks good.

However, one last Kernel test to fix:

 System Listing (Drupal\Tests\system\Kernel\Common\SystemListing)
     ✘ Directory precedence
       ┐
       ├ Failed asserting that file "/builds/project/drupal/core/profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.info.yml" exists.
       │
       │ /builds/project/drupal/core/modules/system/tests/src/Kernel/Common/SystemListingTest.php:38
       ┴
     ✔ File scan ignore directory
    
    FAILURES!
    Tests: 2, Assertions: 3, Failures: 1.

I'm not totally sure this is a bug, not a task, but I don't want to die on that hill. 😅 But if it's a bug, let's smash it.

I pushed a commit to fix the version number in the deprecation message, and also updated the CR. Leaving NW for the release note.

I'm not clear if we should be adding a default version of this to default.settings.php.

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

Do we even need this issue? 😅 Can't we assume that inline comments are allowed in any PHP class, whether or not it's an enum? I'm inclined to say we don't need the whole formal process for this change, and we should configure the sniff to optionally allow for comments. But if we need the formal process for this, I'm formally a supporter now. 😂

Thanks,
-Derek

Sweet, thanks!

I just published the CR ( https://www.drupal.org/node/3486019 → ) and set the "Introduced in version" to 11.1.0-beta1. Hope that's correct. Please edit if needed.

Thanks again,
-Derek

FYI, I opened 📌 [PP-1] Move all system_update_N() methods next to each other Postponed to cleanup a weirdness once this issue is committed to 11.x and 11.1.x.

Whoops, meant for this to be postponed on 📌 Use the new equivalent updates API to prevent updates from 10.4.0 to 11.0.0 Active

dww → created an issue. See original summary → .

Fixed the nits in the 10.4.x MR, then cherry-picked those commits into the 11.x MR. Also did some more cleanup in the 11.x MR. I think this is ready for re-review.

Working on @catch's concerns for the 10.4 MR

smustgrave → credited dww → .

smustgrave → credited dww → .

See also https://git.drupalcode.org/issue/drupal-2630732/-/jobs/3310474 for related fails in BlockCacheTest::testCachePerPage()