🇺🇸United States @dww

Also, we already had the whole discussion about the UI needing "immediate" feedback (which is not true). Nothing about project browser will be "immediate", anyway. The UI already has to handle "waiting to download", so it can also handle "waiting to install". See #3346707-30: Add Alpha level Experimental Package Manager module → for example.

Yes please! I thought we already did this months / years ago? #3346707-18: Add Alpha level Experimental Package Manager module → and later comments resulted in 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed (among others).

There was a request in the #ux Slack channel for a "technical" review. I tried my best. Couldn't help myself from also bikesheding on some of the new UI text. 😅 But I also tried to find any technical concerns.

This is mostly looking really good. Definitely some big improvements over what we've already got!

Disclaimer: I only reviewed the MR diff. I didn't try to apply locally, test in the UI, etc.

Thanks,
-Derek

Re #47: you already can. See #2953489: Add Twig support in Subject, Email, and Message fields →

Major gratitude and appreciation for all the UX/UI help, wisdom and expertise you've provided over the years! I've learned so much from working with you, both on Drupal Core and Drupal.org itself. You'll be sorely missed. Blessings to you on all your future endeavors!

FWIW, exactly this feature already exists in https://www.drupal.org/project/datetime_extras →

Trivial MR.
Issue summary and title match the diff.
Pipeline is green, other than the PHP 8.5 warnings.
Tagging to be smashed.

Nothing else to do or improve that I see, unless we want a follow-up to add something to automatically flag further regressions to using the legacy group?

Possible ways to implement a collective solution:

1. Add another heading to the default issue body template called “Draft commit message”.
2. Adopt the convention that the body of the MR (or at least part of it) should be a draft commit message.
3. Use GitLab commit templates and hopefully get a bunch of the trailers and other goodies automagically per the end of comment #36.

For 1 and 2, core committer copy/pastes from the issue or MR, not the credit UI. For 3, maybe we start merging via GitLab?

Sorry, misread description of “chore”. It’s for non src + test. With build, ci, docs, etc all split out, that doesn’t leave much else a commit could touch. 😂 Maybe “chore” is fine for those cases, and we don’t actually need “task”? 🤔

One possible governance mechanism could be to consider the suggested formatting of git history under the auspices of coding standards. We’ve got an active process for that, it includes core people and non core, etc.

Many / most of our issues are a combination of many of those types. At least test + X. Sometime perf, fix, test, docs all in one. I suppose if we have sufficiently tight scope, some issues could be a single one. But that’s pretty rare. We’d have to assume that fix, feat, perf all imply the corresponding test, docs, etc.

Meanwhile, we use “task” as a catch-all issue category, but we could often be more specific for the commits. So I appreciate the specificity of the list. Yet I wonder if we should use “task” instead of “chore” for Drupal?

#5 looks good for the UI, thanks!

Finally, I’m still interested in hearing about a collective solution instead of something committers have to do entirely on their own. I’m sad we’re proposing to simplify this message so much, only to make the bespoke tooling a little easier, instead of seeing about ways to make this whole task part of the work of getting an issue ready to be committed and then being able to take fully advantage of the improved format for the benefit of out Git history.

Thanks,
-Derek

I wasn’t sold on deviating from conventional commits to put the issue number at the front of the line. My original proposals in the initial issue were all to put the issue number after the colon, as the start of the description, since that’s fully compliant with the standard. So long as we don’t get too creative with the type keywords, and mostly use fix, task and feat, it should still be pretty scannable by humans, and totally readable by tools written for the spec. Even with the issue number the front, they’re not all the same number of digits, so they won’t perfectly line up, anyway.

If you’re looking at a rebase or some other action where you only see the 1-line summaries, it’d be something like:

feat: [#12345] Whatever something nice to say
fix: [#12346] Some other important thing
task: [#1234789] Clean up the whatever

Vs.

[#12345] feat: Whatever something nice to say
[#12346] fix: Some other important thing
[#1234789] task:  Clean up the whatever

IMHO, the 2nd isn’t fundamentally more scannable by humans, but it’s much less scannable by tools written to support the spec.

#34 is not bikeshedding over hypotheticals. Without email addresses (even hard-coding the no reply if we have to), it’s going to really be hard for any other tooling to be helpful, or for GitLab to do its own magic. Can we prevent letting perfect be the enemy of good and do a best-effort for emails, even if there are edge cases where it doesn’t work flawlessly?

Sorry, only on my phone. Hard to closely review. But found a few concerns to start.

Yup, I’ll hopefully get something out next week. Been working at a music camp and offline for a while, but starting to catch up on virtual work now…

Re emails, usernames, etc: sounds like there are some edge cases where we don’t know everything. But a little work could go a long way to providing as much info as we have in the vast majority of cases, no? Eg if I could set my GitLab email address to match the email address I have my local git configured to use, and I always pushed commits with the same address, everything would Just Work(tm), no? Worst case that someone borked this and the same user had 2 different emails? Big deal, we either see both of them or the last one or whatever. It’s still better than having no way to link anyone to a commit other than the Author.

(From the summary):

Any further modifications can be done after the copy/paste of the suggested message.

This is part of the approach to this whole problem that I find a bit problematic. I’d rather this wasn’t entirely dependent on core conmitters to do manually. That’s already a bottleneck. I’m advocating for a solution where at least subsystem maintainers, if not anyone contributing to an issue, can collectively improve this git history in advance. Committers can always override and further edit before they actually push the commit, but we could lighten the load significantly with a collective solution.

TL;DR: I’d rather have no information than wrong information.
-1 to Co-authored-by, +1 to By.

If my heuristic for automatically differentiating co-authors from reviewers in the other issues (if you push commits to the MR, default to co-author, else, default to reviewer) isn’t feasible for technical reasons and/or we’re worried about people debating it case-by-case, MR suggestions blurring the line, etc, I think I’d prefer just By: . Both for copyright and for honesty, I’d rather not call every participant in an issue / MR a co-author. By is bland and vague, just like we use now, but doesn’t imply everyone wrote the change.

When I was originally spearheading this commit format change for core, I was hoping to use it as an opportunity not just to adopt useful 1-line summaries without an incomprehensible wall of usernames, but also to more accurately reflect the reality of who contributed to the change and how. If we call everyone a co-author, it’s less accurate, not more.

Since doing the most accurate things automatically are revealing some technical hurdles and other resistance, the other approach I keep pondering / suggesting is to put the draft commit message to use in the MR body and/or as another section of the issue template and let folks craft / refine the draft commit message collectively. We already have a culture of encouraging people to keep the issue summary accurate. We already have a “field” in the template to collectively write a release note. However, I know folks don’t always do this. Core committers could start refusing to commit RTBC things if they don’t have this message already written. We do that for CRs and release notes. We’ll delay issues for weeks/months nit picking code comments, but then we leave the urgent task of the Git history entirely up to committers to have to do on their own.

The other option, if relying on MR/issue text isn’t going to fly, would be if the credit UI saved state and we could draft a “better” message via that UI. But that’s probably a whole other can of worms.

If there’s no draft, we need a quick / easy way for committers to get something useful, which is what this issue aims to provide. So if every attempt at reflecting reality is going to be ruled out, I vote for something vague and true, not specific and (partly) false, even if GitLab would provide some limited “magic” automatically if we used Co-authored-by.

If folks don’t want this to reflect reality, then I’m opposed to anyone being called “Authored-by”. If we’re only allowing a single “role” it should be something generic and bland like
“Assisted-by” or “Contribution-by” or something. But calling all creditable participants an author is really problematic IMHO.

In terms of time spent: if the credit UI saved any state, subsystem maintainers could help craft the right commit message during RTBC.

Fallback would be to draft the right commit message as the body in the GitLab MR description. I do that on some issues, but core committers never use that.

Also, the heuristics I proposed above would cover 95% of the work, I think. If you push any commits, you’re a “Authored-by” user, otherwise, “Reviewed-by”.

Finally, I’m strongly in favor of spending a little development time on this feature so that it’s easier for core maintainers to get this right. We can’t seriously claim that a few hours of DA dev time (once) are more precious than dozens / hundreds of hours of committer time each year doing it manually.

smustgrave → credited dww → .

Sweet, thanks!

Thanks! Doesn't look like you actually pushed the commit to 11.2.x. This issue doesn't show it, nor do I see it in Git.

Super nit for the sample code, not related to what’s being fixed…

@return array[]

Kinda weird. Probably we want either just:

@return array

Or perhaps something like:

@return string[]

or whatever. Not that we don’t have functions in Drupal that return nested arrays. 😂 but we’re trying to move away from ArrayPI.

Yeah, this didn't apply at all. Had to re-implement it in system.install (different indentation, etc, etc).

https://git.drupalcode.org/project/drupal/-/merge_requests/12988 for 11.2.x and lower.

https://git.drupalcode.org/issue/drupal-3538854/-/pipelines/572272 is basically looking good. I assume we ignore the PHP 8.5 phpunit deprecation warnings for these backport MRs, right?

Back to NR. Probably worth another set of eyes to look at this before the backport is RTBC.

Great, thanks! I'll work on a backport MR right now. Stay tuned.

If getting everything via API is a pain, can we iterate through all the commits, and for every one, whatever is in Author: we add it to a list and add all unique values as Authored-by: footers in the default commit message? Not as ideal, but seems like it’d cover the 80% case really well with no data beyond the commits in the issue forks.

I need to make time to closely review this and help it along. Meanwhile, #2559313 is closely related.

Trying to make progress while we wait for 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active to land.

The old branch had massive conflicts when trying to rebase it on top of #3538637. So I opened a new branch, added a single commit for #3538637, then 2 commits for the merged results of that and the previous MR branch, 1 for the change to set the subject, one for the test coverage. Once 3538637 is in 11.x, it'll be easy to interactive rebase this to remove that 1 commit, then we'll have an easy 2 commits here to review...

dww → changed the visibility of the branch 1818764-correct--improve to hidden.

@bramdriesen: Yes. E.g. see https://new.drupal.org/contribution-record?source_link=https%3A//www.dru... at the bottom of this issue. Big blue "Contribution record" button.

There are some more tweaks that would really help this be fully functional. See https://www.drupal.org/project/issues/contribution_records →

🌱 [META] Improve sspport for semantic versioning for extensions (modules, themes, etc) in Drupal core Active

dww → created an issue.

Reading summary and MR, I wonder if a setting fallback (instead of having to install it via composer or adjust the PATH) for finding composer would also make sense. Seems easier to migrate from the config to a setting than the other choices. I don’t see how it’s worse from a security standpoint. We already have to trust settings isn’t writable by malicious actors.

+1, I was just looking for exactly this setting today and getting confused about why it wasn’t visible.

Thanks,
-Derek

+1. For folks who push commits to the MR, yes.

I also opened an issue so that even if I’m credited with reviewing something that I still get identified with my preferred email.

Thanks for opening this,
-Derek

Re:

Not sure the cleanest way to handle "Committed-by", but maybe that "needs" to be here, too?

.

I guess we'll always have the formal Git Author header for that, so it doesn't need to be duplicated in the commit message body if we don't want to mess with it. So long as core committers never push commits with --author shenanigans, we're fine. Although, bummer, I just did a grep of the core commit log:

git log  egrep 'Author: ' | sort | uniq -c | sort -r

The results are attached. There are approximately 30 that I recognize as core committers, and maybe 150 unique authors that must have been "forged" on commit before pushing. I think d.o separately had/has a notion of who pushes for credit and issue updates, not just the --author value. So probably we shouldn't rely on Author, and it'd perhaps be safer to stuff "Committed-by" (or perhaps "Merged-by"?) into the footer, too?

I should add: all the energy around Drupal CMS as the primary "product" that folks start from, which now includes both project_browser and (new) update_manager, means anyone starting out with Drupal now will have a much better experience in this realm "out of the box". So the existence of drupal_cms itself is another reason this issue is now outdated.

Finally, I must also add, the UI/UX for the D7 "update manager" used to be *much* worse before Bojhan started helping me fix it. 😂 I learned so much about usability through those efforts, and (pre-composer) core "update manager" was vastly improved by it from D8-D11. We definitely didn't fix everything, but we made real progress. I'm eternally grateful to all the time @Bojhan spent on those issues and efforts, for their generosity sharing their knowledge and expertise, and for their patience while I (and others) tried to implement and refine their suggestions. Formally crediting @Bojhan here for the issue write up, linked issues, etc.

Deep thanks!
-Derek

Bojhan was recently removed as a UX maintainer for core. See 📌 Remove Bojhan as maintainer for Usability topic Active

Although this is classified as "extension system", it's really tightly coupled to "update.module", of which I am still an active subsystem maintainer. 😅

I believe it's safe to close this. Most of the stuff he's talking about improving has been removed from update.module. It's no longer called an "Update manager". It's being replaced by both project_browser, the new "update_manager" (what everyone still calls "auto_updates"), and for the non-UI among us, by the fact that Drupal requires using composer now, which simplifies (and complicates) a lot of this UX for us.

If

Thanks, these are all great improvements!

Instead of reopening, I spawned a follow-up: ✨ Add a UI to change footer "token" values for each user Active

dww → created an issue.

Is there some setting in GitLab where we could specify that we'd prefer to use a real email address for these, instead of the no-reply ones? If so, would it be possible to include both the Git username (which maps to the d.o username, no?) and the email address?

Authored-by: dww <git@dwwright.net>

Not just:

Authored-by: git@dwwright.net

?

Thanks!
-Derek

FYI, 'task' is widely used for conventional commits, and it maps to one of our most regularly used issue categories. Instead of calling this field "Task type" Could we call it "Commit type" and leave "task" as one of the possible options?

Speaking of which, would it be easy for this UI to default this field based on the current value of 'Category' in the issue? Or is that wasted effort as we're migrating to GitLab issues? Happy to spin off a follow-up, if desired, or to drop it.

This is my first time seeing the new UI. I'm thrilled we've got a tool to help us construct better commit messages now! Huzzah.

Sorry to reopen this, but the new sort order still seems not to match the ordering from the d.o. For example:

https://www.drupal.org/project/drupal/issues/3538854 🐛 APCu requirement for 32MB always displays since APCu 5.1.25 Active
vs
https://new.drupal.org/contribution-record/11419172

(See attached screenshots)

I believe the new is now sorted chronologically by each user's first comment in the issue.

1. It'd be great to at least get back to the original heuristic.
2. Better yet, it would take GitLab MR review threads into consideration, too (not just that pushing commits == "patches").
3. Ideally, whatever the default order was in the UI, there's be a drag-and-drop for the rows in the UI so we could reorder when the heuristic gets it wrong.

Happy to split off follow-ups for any/all of this, but I'm reopening since it seems like at least point #1 was the original intention here and it doesn't seem to be happening.

Thanks!
-Derek

Tagging to be smashed.

Also, crediting @driskell for the clear bug report with correct analysis of the bug and accurate proposed resolution. Had @godotislate and I seen this issue while we were scrambling to fix 🐛 Incorrect warning for system requirements for APCu memory Active it would have saved some time, since we had to debug why the status report thought 3GB < 32MB and only with a little debugging output did we discover apc.shm_segments was 0 and then search to find it had been removed upstream. 😅

Pipeline here is mostly green, except for the dreaded package_manager build test fails. 😬 Queued that one to re-run. But this should be ready for review (and hopefully RTBC) now.

Thanks,
-Derek

Agreed with #29. It'll be cleaner for the Git history, too. I opened an MR over there with the latest code from here. Restoring the original (critical) scope and marking fixed.

Thanks,
-Derek

Opened MR here, with the code from https://git.drupalcode.org/issue/drupal-3539331/-/tree/3539331-fallback-...

Since both myself and @godotislate worked on that MR, adding credit for them, too.

I’ll work on moving over the code and cleaning up the other issue, stay tuned.

Indeed, the other issue started as a critical due to repeated test fails on HEAD. We committed a stop-gap to skip that part of the test. There’s now an MR there to fix the status report, too, that needs review. But probably best for scope management and history to move that MR here and close that one as the hot-fix-the-tests issue it started as.

You’re welcome and thanks for the review. Since this task is blocking a bug fix, tagging.

smustgrave → credited dww → .

I've been over the MR multiple times with a fine-toothed comb. All but one of my concerns have been addressed, or at least follow-ups opened. The remaining open thread could also be a follow-up, it's a very minor nit about defgroup and I'm not entirely sure how relevant those tags even are anymore. But I'll leave it open for a committer to review before merging this.

Latest pipeline is mostly green. The Validatable config job is failing, but this MR isn't changing anything about config validation, so I don't think it's worth investigating why that job failed.

I could probably come up with some more hyper-pedantic nits if I really tried, but I'm trying to relax that approach. 😂 Progress is better than stalled perfection, so let's get this in and move on.

Thanks!
-Derek

Thanks for opening this.

This is really quite close. Opened some more MR nits. I don't see anything else to improve in here.

Also, edited the CR since this is 11.3 material at this point.

Initial pass at saving credit:
@mcgovernm for (almost) all the code
@nicxvan for heavy reviews, the rebases, and some of the code
@berdir and myself for detailed MR reviews

At this point, none of the other commenters here seem to reach the level of "creditable contribution", at least to my eyes.

Technically, I believe the problem here is the lack of a project info key, not version. If that assumption is true, 📌 Parse composer.json for the project name for update module Needs work would fix it.

Saving credits.

Tentatively agree this is RTBC, although I couldn't resolve the 2 open threads in good faith, and I even opened a 3rd one.

Everything else in the MR looks good to me. Very straight-forward conversion. Made a few tiny edits to the CR for formatting, but that also looks good and is clear.

Saving credit for code and reviews.

Just did a close review on the MR changes. Agreed this is RTBC. I resolved a couple of the open threads. Left a micro-nit on 1 open thread, but not worth changing the status over it.

However, do we need a CR here to introduce the new enum?

Thanks,
-Derek

There are 2 legit unresolved MR threads. I want to take this out of RTBC, since it’d be better to resolve those before a core committer looks at it. But we probably need a framework manager or release manager to decide on the BC break problem and how to handle the API change to NodeTypeInterface::getPreviewMode(). 😅 So I’m leaving the RTBC, but won’t formally tag it for any specific sign-off (yet).

Thanks/apologies,
-Derek

Saving credit for reviews.

Thanks! Left some review comments. A few just comments / musings, a few of them are things we should fix before we merge this.

That'd certainly be nice. I guess we're saying you can't upgrade from 8.x-1.x straight to 3.0.x. But that's true, anyway, since the 8.x-1.x branch only supported D8 core, and there's no way to get a site like that directly onto 3.0.x. Folks would have to go through 8.x-2.x for "bridge releases", anyway.

So in principle, +1.

However, the MR pipeline is failing. Haven't looked into why. NW for that.

Thanks,
-Derek

dww → changed the visibility of the branch 3539331-fix-apcu-status-check to hidden.

Hah, bit of duplicated effort, since I had started down that same road.

However, it's not just ini_get('apc.shm_segments') that can be empty. So, too, can apcu_sma_info(TRUE)['num_seg']. So I moved the fallback check into a local var, and use that everywhere we check this, either directly or via apcu_sma_info().

Thanks for your interest in this module and your offer to help!

However, I've never seen you contribute to this module before, other than a single comment in the D11 porting issue. I have no idea about your skills, abilities, your understanding of semver, nor your approach to maintaining projects. Do you write phpunit tests? Would you require them before committing changes here? How collaborative are you? I don't know what kind of a coder you are, how careful you are, your approach to using Git, issues, semver, your philosophy about release frequency, etc, etc. That's a lot of unknowns. 😅 I don't assign co-maintainer status to people I don't know and trust.

If you're interested in being a co-maintainer, I need to see more from you: some actual code, or at least detailed reviews of other people's code. 😂

Initial MR to skip the test for now.

Postponed means it's blocked on something. This isn't blocked, only awaiting interest and available time. I'm interested, but not available. 😂

Bah, sorry. I hadn't looked at the ini_get() docs about apc settings. apc.shm_size indeed can come back as "32 MB" and friends, which is what we want to convert to a number before we multiply it by the number of segments.

Opening an MR to try that

Misplaced parents?

Bytes::toNumber(ini_get('apc.shm_size')) * ini_get('apc.shm_segments')

Don’t we want:

Bytes::toNumber(ini_get('apc.shm_size') * ini_get('apc.shm_segments'))

?

Adding 🐛 Support "View any unpublished [entity_type]" and "Administer [entity_type]" permissions Postponed as related, which itself has a bunch of related issues that are relevant here.

There have been a lot of efforts along these lines. Adding a bunch of (very) related issues.

Let's see what the bot thinks of that. 😅

@bluegeek9: Thoughts? Any objections to removing local variables in this case?

IMHO, #11 is a better approach. Why have the local variables (that need special comments) at all?

There has been no solution to this problem. It's a UX improvement in a somewhat obscure feature of core that I imagine is relatively rarely used. But the current feature still has the same problem as clearly described in the issue summary. If you want to attach a rendered entity to a Views area (header, footer, etc), you have to manually type in the entity ID. See attached screenshot. This is not intuitive. An autocomplete would be better.

Love this. But curious how it would work in the Field UI. Is the idea that if a field uses this to manage allowed values, that those parts of the UI where you define the values and labels would disappear or something?

Thanks for working on this! Opened a bunch of MR threads. Mostly very nit-picky bikesheds. A couple things of substance.

Re: #29 - Right, that's the basic problem I foretold at #12. The backend needs to know about this, too. If the behavior is to be determined by field cardinality, we need some way for the Field API (which knows about field cardinality) to communicate with the Form API (which handles autocomplete) about how to handle commas.

Thanks for the review!

Good question about the other statuses. "need" is a strong word. 😂 I'm slightly worried that every possible combination of all possible status values for core vs. contrib will explode the size of this data provider. It's not like the logic in _update_message_text() is all that complicated. Seems plausible that we don't need to test every possible code path in there. This MR is already larger than the "combined" MR that prompted splitting this off to a separate issue. 😅 I don't want to explode the scope here more than I already have.

I vote we get this in ASAP as a significant improvement to what we have now (and to unblock the bugfix that prompted this). If we decide we need more cases later, we can always add them easily enough.

Thanks,
-Derek

Re: package vs. project vs. module vs. theme vs. extension:

1. A project (or "package") is a collection of one or more modules or themes.
2. Each module or theme is an "extension".
3. "Project browser" is browsing projects, not individual extensions.
4. Ditto package_manager: it's operating at the level of complete composer projects/packages, not individual Drupal extensions

So I think we've landed on (more or less) the right names:

• package_manager
• update_manager
• project_browser

Arguably, "project_browser" could become "package browser" or even "package finder", but per #14, "project" isn't any more of a Drupalism than "package" would be, and "project_browser" is already the name in a bazillion places (docs, presentations, existing code, etc).

Whereas:

• All 3 of the new core modules now have decided names.
• I posted over 6 weeks ago proposing that this is fixed, and there's been no further discussion or concerns.

Therefore be it resolved: I'm going to go ahead and mark this fixed.

Initial pass at saving credit for meaningful contributions here. If anyone feels left out, feel free to add a comment.

Thanks,
-Derek

Anyone following this issue: 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active is now ready for review if anyone is willing to give it a look. Thanks!

I believe this is ready. The test is passing locally, so if the pipeline isn't green, it's due to a random fail. 😅

Per Slack chat with @xjm, postponing this bug fix on 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active , which I've re-scoped into fixing UpdateMailTest before we try to expand it here.

Per Slack with @xjm, expanding scope here to actually fix UpdateMailTest on its own, before trying to add more coverage to it at 🐛 Correct & improve update status email subject Postponed .

Thanks. A few thoughts:

1. Agreed we shouldn't be trying to port 8.x-2.x to D12.
2. Personally, I wouldn't worry about trying to port anything to D12 until at least 12.0.0-beta1 is out. "Chasing HEAD" is a nightmare. I wouldn't be trying to follow bleeding edge core changes as they happen. I'd wait until we were sure the D12 API wasn't going to change, and then worry about porting.
3. Between now and when #2 becomes real, running this job on every push to every issue fork, and on every upstream commit, is a waste of time and resources. So I'd remove this from the 3.0.x branch, too, and run the job manually when we're actively porting to D12.
4. Declaring we're already compatible with D12 is a lie, since we have no idea what other changes will be needed to actually work with a 12.0.0 release.
5. It's helpful if the Git history is accurate. Too bad that you reverted the change on the 8.x-2.x branch with the message: "Issue #3538409 by bluegeek9: CI Pipeline: Add upgrade report". You already added the report, this commit was to remove it. 😂 Please don't auto-pilot with the crappy d.o commit messages. 😅 It's best to say what you're actually changing. In this case, "revert" would be a good word to include. Frankly, instead of a new MR and all that noise, I would have run git revert cf9ba43b26d at the CLI and pushed that.

Meta: I know you were added as a co-maintainer here since I'm chronically busy, over-committed, and don't have enough time for this project on a regular basis. That said, it'd be nice if you waited more than a few hours between opening issues and making commits. 😅 I believe that nothing is so urgent here than waiting a day or 3 would cause any harm. Would you be willing to treat this more as a collaboration, allowing time for myself (and anyone else interested in this project) to weigh in on issues and MRs you open, instead of directly committing them as if you're managing a project all on your own? Thanks!

Okay, did some more sleuthing, and decided that 2 of the existing cases in this test are totally bogus, as is 1 of the new ones we added:

• _update_cron_notify() is the only place sends the 'status_notify' email.
• It returns early if there are no values in the $params array.
• It therefore seems pointless to unit test our hook_mail() implementation without parameters.

Removed all 3 cases, and the related @todo comments from my attempt to document everything.

Apologies for the possible scope creep. Perhaps we should postpone this on a new follow-up just to fix all the brokenness in this test, then circle back to fix the bug and expand the test. But it seemed more prudent to fix them all in 1 shot.

With the draft CR done, all MR threads resolved, and a green pipeline, this is ready for review again. 😅

Initial CR: https://www.drupal.org/node/3538659 →

This is going to conflict with 🐛 Correct & improve update status email subject Postponed , let's land that, first.

This is getting close. Pushed a few commits to resolve a few more of the open threads. Down to 1 (the request for inline comments describing each case in UpdateMailTest::providerTestUpdateEmail()).

Also, opened 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active which I noticed while reviewing this. That's totally out of scope here, but should happen.