🇺🇸United States @dww

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()

Re: #208: Yeah, I'm also seeing a lot of fails for ImageStylesPathAndUrlTest recently. Opened 🐛 [random test failure] ImageStylesPathAndUrlTest Active for it.

dww → created an issue.

I believe this is a more appropriate component for this request...

+1 subscribe 😂 (inside joke for truly old-timers).

I should have added: the new docs look great! Big improvement. Thanks for working on this!

Spotted one problem with existing docs right next to the ones we're adding in core/tests/Drupal/Tests/UnitTestCase.php. Should we fix those here or in a follow-up?

I think I fixed the tests. Let's see what the bot thinks.

Meanwhile, crediting @quietone for the initial work, @catch for the detailed reviews + input, and myself for trying to fix the tests...

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

Retroactively crediting @mstrelan for all the work (issue, automation + MR), and @bbrala for the review.

After having only looked at the MR, I've now read the issue and comments, and see that my concern about the logic change was explained in #17. I'm on the fence if I agree with it. But I don't want to die on that hill. If we want to start showing the "Coverage has ended" error as soon as you hit the "coverage ends on" date, that seems fairly reasonable, even if it is a bit of scope creep.

Thanks for #17 also explaining why we're adding TestTime:: getCurrentTime(). I started down the path of re-debugging why that was needed, but got distracted by the other changes and forgot to open a thread about it. 😅 It might be nice to add a comment about it, but I certainly wouldn't require it here. Could always be a follow-up if we're anxious to get this committed ASAP.

Thanks again,
-Derek

Thanks for working on this! These tests and the logic they're testing are pretty confusing and weird. I tried to review as best I could. I think there are some errors in the MR as currently written. Although I'll quickly grant I could be confused myself. 😅 Anyway, I think this needs another look before it's RTBC. Bare minimum there's a faulty comment to remove (nit). But in addition to changing the dates, the MR is also currently changing the behavior of the requirements messages, and I'm not sure it should be doing that. Locally, by fixing a few of the mock dates in UpdateSemverCoreSecurityCoverageTest, everything passes with no logic change in core/modules/update/src/ProjectSecurityRequirement.php...

I believe core committers still don’t use GitLab for merging MRs. They have their own local scripts and end up applying everything as a patch locally, running some pre-commit checks, and pushing the resulting commit.

Ideally, “the tooling” would make it easy to make these readable commit messages for everyone, not just core committers. The default commit message in the d.o issue UI is dutifully implementing what has been our convention for years, but it’s making out git histories really awful, hard Ti read and harder to use / understand. The goal of the policy issue was to get core to decide a new convention with the explicit hope that most / all of contrib would follow their lead.

Step 0 could be to modify the d.o issue hit message UI to stop injecting usernames at all.
Step 1 could be to fix it to use the [#xxxxx] $type: $title convention
Step 2 could be figuring out how to implement this in GitLab with as much automation as possible
Step 3 could be to understand why core committers still have a patch-based workflow and see if we can unify that with GitLab workflow
…

I’m not sure what queue is most appropriate, but I’m not sure it really matters. Maybe we’re not going to spend another minute on the d.o issue UI’s commit message tooling. Maybe this is entirely a GitLab configuration thing.

But I hope it can be fixed in a way that either makes life better for all of contrib, or at least is extremely easy to reuse across projects so contrib can trivially opt in.

Thanks!
-Derek

p.s. I believe the test failures are random and unrelated to anything I touched here, but I haven't been closely following core's random fails these days so I'm not sure:

https://git.drupalcode.org/issue/drupal-3483501/-/jobs/3164942

Drupal\Tests\system\Functional\Theme\ToolbarClaroOverridesTest::testClaroAssets

https://git.drupalcode.org/issue/drupal-3483501/-/jobs/3164948

Drupal\Tests\block\Functional\BlockCacheTest::testNoCache
Drupal\Tests\block\Functional\BlockCacheTest::testCachePerPage

I opened an MR to get started on #3483501, but there are a bunch of open questions in the issue for release and/or framework managers to weigh in on. Thanks!

Also note, I'm not touching any of the legacy "Update Manager" parts that update contrib in place. I'm not clear on the order by which everything should happen. Ideally, this issue, an issue to deprecate / remove all that stuff*, and ✨ Add Alpha level Experimental Automatic Updates module Needs work all land in the same minor release of core.

* 🌱 [meta] Deprecate tarballs, because they are incompatible with Composer-managed dependencies, Automatic Updates, Project Browser, and release managers' health Active points to #3201968: Augment then Replace current Update Manager URL download based updates with Staged-Composer workflow → , but maybe that's no longer the path forward. Perhaps we want a new clean issue about deprecating and removing the "legacy update manager with authorize.php" (for lack of a better term, wow this is going to be a confusing transition). 😅

I gave this a start. To do this properly is definitely not novice material, removing that tag.

Here's a TODO of bigger fish I'm punting on for now:

1. This one is sorta problematic: Drupal\update\UpdateManagerInterface. Handle that here, or split into a followup? Could probably be renamed to UpdateStatusInterface, but unclear about the BC implications of that.
2. update_help() says this:
   

   The Update Manager system is also used by some other modules to manage updates and downloads; for example, the Interface Translation module uses the Update Manager to download translations from the localization server.

   Is that still true? If so, yikes. 😉

3. Docs updates at https://www.drupal.org/documentation/modules/update → -- follow-up, or resolve via the tag here?
4. Renaming / re-documenting tests/*

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

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/updat...

name: 'Update Manager'
type: module
description: 'Checks for updates and allows users to manage them through a user interface.'
…

To vote on my own poll from the previous comment, I’m totally fine with option #2 and changing the human readable name of update.module back to “Update status”, add CR and release notes, fix documentation, and hope for the best that it won’t be too confusing. But I don’t personally have time or any funding to do all that work myself. so if y’all decide that’s best, I’d appreciate some help getting it all done. Thanks!

+1 to not calling the new thing “auto”. Side note, +1 to make it so it can be configured to only update when a human says so, and never “automatically”.

However, for what it’s worth, “Update Manager” is already the human name for update.module in core. Once upon it time (D5 contrib) it was called “Update status”. When moved into core in D6, Dries wanted it called “Update Manager” since he had dreams of it actually updating your site. Those dreams were (partially) realized in D7 when we added all the authorize.php stuff and contrib updating. The machine name is still only update, but help text, .info.yml name, and reams of documentation and countless issues & comments all call it “Update manager”.

So, if we go forward calling the renamed auto_updates “Update manager”, we have to pick from:

1. We’re okay calling two different modules with different responsibilities the same thing. 😬
2. We should rename update.module back to “Update status” in UI text and documentation (#NeedsFollowup). Once this lands, and we finish ripping out the authorize.php parts, providing “status” is all update.module will be responsible for again.
3. Or we pick a different new name for auto-updates.

This is no longer postponed on anything, so removing the "[PP-2]" prefix. Curious about how this unfolds.

I'm basically in agreement with #13, although one underlying problem is that this existing interface isn't well documented / defined. So even if we "leave it alone", I support trying to improve the docs and clarify what it was supposed to mean. 😅

But yes, perhaps adding a new interface (with clear docs and scope) for "internal updates" or whatever would be prudent.

Thanks,
-Derek

Happy to open another issue if appropriate. I think we should expand the scope to allow these tags anywhere d.o allows filtered HTML. Comments on issues (and issue summaries) are two other spots where they would come in very handy (although I guess we have to make sure those would still work once we migrate to GitLab issues). Project pages themselves could also benefit...

Wheee, that pipeline failed on basically every step. 😉

• cspell
• phpcs
• phpstan
• phpunit (current)
• phpunit (previous major)

This isn't yet declared compatible with D11, so no surprise that phpunit (current) is failing...

Trivial MR up to get this started. Let's see how https://git.drupalcode.org/project/views_taxonomy_term_name_into_id/-/pi... goes...

dww → created an issue.

gábor hojtsy → credited dww → .

Thanks for this! The MR looks good to me. It's great we can maintain D9 compatibility, so no need for a new minor version or anything.

Do you want to commit this and cut another alpha, or shall I?

Cheers,
-Derek

#51 I hear you that the component can usually be inferred by the summary text. But the promise of conventional commits and related tooling is that if you fully standardize on it you can actually grep the history to learn things, make release notes, etc, instead of parsing text and having to “understand” it.

Thanks, y’all! Indeed, I’m not angry, and still have my temper right here by my side where I left it. 😂

To be extra clear: I agree that contrib will hopefully follow core’s lead on this, especially if we work out tooling to make it easier to get this right. But contrib can already do whatever they want. I’m acknowledging that freedom, and I’m not claiming this issue will mandate contrib to do the same.

The thing we can change is the format core committers use when they push to the canonical core repo. Once that’s better (and easy), I imagine most of contrib will do the same.

Thanks again,
-Derek

Re #40: 100% correct. This is for the core Git history, the result of the squashed merges that get pushed to the canonical core repo.

Re: #39: No one cares about intermediary commit messages in MRs, neither core nor contrib. Those aren’t “history”.

p.s. Re: #33 and option E: If your concern is that Category and Component can change in the issue but not in the Git history, why put them in the commit message at all (even if only in the footer)? That seems to undo the whole point you were trying to make at the end of #32. I think the "optional scope" here has been a distraction, which is why I dropped it entirely in option D. "Category" is valuable, which is the premise of conventional commits. See #35.3. But option E seems to have all the drawbacks and none of the benefits of putting the category directly into the 1st line summary.

1. This is a policy issue for core. We cannot and do not enforce what happens in the "Wild west" of contrib. Yes, many/most contrib maintainers follow core's lead on such things. Certainly the bespoke tooling on d.o issues is largely to "blame", since folks do what's easy. But it's entirely a distraction to worry about differences, enforcement, wringing our hands over how hard it will be if contrib maintainers continue to do their own thing, etc. I just want to be able to read the core Git history and understand WTF is going on, which is close to impossible with the current state of things. Please let's focus on that problem, and not delay this another 10 years (yes, the efforts to fix core's commit conventions have been going on at least that long) by worrying about contrib. If core starts doing this, contrib will (hopefully!) quickly catch up...
2. This may or not happen before we migrate entirely to GitLab issues. Either way, "we" are almost certainly not going to be changing any drupalorg.module code to better support this. Sadly, there's likely to be some manual effort for crafting the right attribution footers for core commits. That's probably a deal breaker, but I/we can still dream. If it comes to it, we'll have to drop the attributions. Or hopefully there are ways within GitLab to make that possible/easy. I haven't hacked around enough in GitLab to know. There's a chance that GitLab has templates and variables that might be sufficient. So I don't think we need an elaborate bikeshed discussion on the exact formatting of the footer, since all that might have to be dropped for expediency, anyway.
3. I've only said it dozens of times, but I'll say it again: "Issue #xxxx" is a waste of bytes. What kind of issue? That's part of the point of conventional commits and semantic versioning -- if we see "feat" in the Git logs for a given tag, it should be a new minor version. If we only see "task" and "fix", we know it's still viable for a patch release. Yes, people can / do get in shoving matches over bug vs. feature, and the d.o fields can change. Same with credit + attributions. Things change, but Git history does not. That said, I'd rather the Git history did the best it could to reflect and document what we thought was true at the time the commit was pushed. I don't think it's the end of the world if someone goes back and retroactively changes an issue from a task to a bug but the Git commit still says task.

Back to NR. I don't see anything of that warrants needing more work here. We need to keep reviewing the options, and ideally converge on agreement. NW will doom this to further delays and more nearly useless core commit messages... 😅

Thanks,
-Derek

Done. Thanks for the suggestion.

We already had 'Site structure'. Seems fine to add 'Content display', I guess.

Defined the summary per #3, but moving to NR for other eyes before I close it.

Actually, what’s wrong with the existing summary?

Provides functionality for storing, validating and displaying international postal addresses.

?

Thanks for opening this. “New” seems useless in this description. The whole thing seems wordy. How about:

Provides a field for storing international postal addresses.

?

• Removed the duplicate B from proposed resolution and set that to “TBD”.
• Added a little clarification to D for context.

P.s. I’m -1 to trying to have GitLab auto close issues based on commits. I don’t think that’s a useful goal for us, especially in core. We often need at least 2 different MRs for any given change depending on backports. There are change records to review and publish. While auto-close might make sense for smaller contribs (who can already format their commits however they/we want), this issue is for core, and I don’t think we ever can rely on auto close just because any single commit was pushed.

Re #26: that’s exactly option C.

But folks are worried about “optional scope”. I tend to agree it’s mostly going to be noise, often duplicate with the summary itself (Eg the summary should tell you basically what part(s) of core are changing), and if you really need to know, we’ll have it in the diff itself and the linked issue.

That said, I’d be happy with D, C or A. Anything but B. 😂

Huge -1 to B. As I and others have said, the only things I REALLY care about in the first line are the issue ID and a short, meaningful summary of the change.

I also don’t really care about conventional commits as such. However, I think (task|feat|bug) is vastly more useful in the summary than “Issue”.

If the “scope” part of conventional commits is considered noise, let’s drop it. That was always intended to be optional. Added option D to the summary, which is basically #8.

I’m very strongly in favor of D over B.

Thanks,
-Derek

Nearly 2 months later, still no explanation of what this is actually wanting to accomplish. As update.module subsystem maintainer, I agree with @longwave in #27 that from what I can tell of what's intended here, it's not something we want in core. Once upon a time (D7), https://www.drupal.org/project/update_advanced → allowed sites more options to configure what got reported. There are a bunch of hooks in core that allow such a contrib module to modify what update.module in core sees. If someone really wanted a "news mode" (?) for their available updates report, they could implement it in contrib.

Therefore, closing this as "won't fix".

Thanks,
-Derek

larowlan → credited dww → .

Agreed with #3 -- isn't this fully duplicate with #3309010: Support PHPDoc Types in @param @var @return annotations → ? Can we close this and focus efforts in one place?

This was already reported at 🐛 Missing description in FieldType declaration RTBC , which already has the change as an MR. Let's solve it there.

Thanks,
-Derek

git blame in the 3.0.x branch points here:

commit d0599616617e0dcbda56febb2922ff6387b186ac
Author: Sahil Goyal <ketangoyal1042@gmail.com>
Date:   Tue Apr 11 00:26:28 2023 +0530

    Replace function to fix undefine func error

Good it was fixed. Bummer the commit didn't use an issue number, no reference to here, etc. 😅

Guess we can call this issue fixed...

Cheers,
-Derek

I just upgraded the site where I need this patch to 10.3.x and discovered we’re using yet more deprecated code in here. Opened an MR with patch #15 ported to D10.

dww → created an issue.

1.0.0 is out. Thanks! Saving credit. 😂

Unfortunately the changes to the form elements and values makes this a more consequential and potentially breaking change. Eg anyone altering these forms will have to update code to match.

Probably better for now to ignore those spellings and open a follow up to rename these form elements with possible upgrade path if needed.

Otherwise, looks great.

Thanks!
-Derek

Merged.

Cool, https://git.drupalcode.org/issue/media_stream-3466776/-/jobs/2375830 shows the functional JS tests passing on an 11.0.x site.

dww → created an issue.

Okay, this is ready for review again. There's a lot of noise in the GitLab pipeline output, since the baseline 2.1.x runs are full of warnings. 😅 I fixed the new phpstan error introduced via this patch, but otherwise, I didn't touch anything to get the rest of the warnings green, since that'd all be scope creep in here.

I've also been successfully using #42 for a client site for quite a while. It'd be great to get this in. There's now a 2.1.x branch, but sadly no 2.1.x version to use in issues. I'll convert this to an MR that merges cleanly to 2.1.x to hopefully move this forward. Stay tuned.

This can now be closed, right? 🎉

Thanks! 🙏
-Derek

hestenet → credited dww → .

hestenet → credited dww → .

hestenet → credited dww → .

hestenet → credited dww → .

hestenet → credited dww → .

Thanks! After getting GitLab CI going and adding some test coverage, this was ready to merge. I'll cut another release soon.

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

https://git.drupalcode.org/issue/media_stream-3464620/-/pipelines/246228 shows this works with 9.5.x.

This will make more sense in the default pipelines when I merge 📌 Automated Drupal 10 compatibility fixes RTBC

Merged to 1.x

Re: multiproject issue queues: Okay, cool. P.s. those will be changing when issues move to GitLab, possibly goi g away? 😅

Main point: completely agree with this:

I'd rather we spend resources on completely voiding credit gained through spamming issue queues

You’re welcome. Glad to help out. Thank you!

✨ Make email not required for a Drupal site account Active

Is this not duplicate with 🐛 Email Address should be a required field in the "Add User" form Needs work ?

While I sympathize with the pain that's driving this proposal, -1 as written/formulated.

I agree with basically all of #2, except I don't quite follow this:

we should either get rid of or limit access to the generic issue queue

Do you mean https://www.drupal.org/project/issues → and https://www.drupal.org/project/issues/search → ? How else would anyone be able to search for all issues tagged with a certain tag across all projects? I don't think removing or restricting access to those pages makes sense. If companies are having their employees circle the skies like vultures for anything to hit 'needs review', that's unfortunate, but I think we need a cultural solution, not a technical one. Removing the ability to search issues across projects seems not worth whatever perceived benefits of discouraging this form of "drive by contribution"...

dww → created an issue.

Got really slammed with many things. Finally circling back to this. Upon some of my testing, I was greatly confused as to why there weren't fatal errors if you updated your views config to point to plugins that don't exist. The answer is there's a bug in core 🐛 Views handler loading should respect configuration Needs review . 😂 Views doesn't actually honor the plugin IDs in the exported config, and instead uses the whole views data API to determine exactly what plugin to use for every specific field on every specific table.

So, due to #3458099, I was totally wrong above. 😅 Until 3458099 is committed, it's actually completely harmless to "fix" your config and backport it, even to branches where the new plugin doesn't exist. So nearly all the complications in this MR are unnecessary and all you really have to do for now is update your views config. Nothing will actually use that config to determine what plugin is used until 3458099 is fixed. All that actually matters is the views data API implementations in each branch, and those will always find the correct plugin based on what exists.

dww → created an issue.

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

dww → created an issue.

Mostly looks great, thanks! This is very close to
ready. However, I opened an MR thread about one of the new tests that I think should be addressed before this is RTBC.

bbrala → credited dww → .

In case it was missed in the middle of #29:

Should we backport the deprecations in core/lib/Drupal/Core/Updater/*, too? And change them to being deprecated in 10.4.0? Or leave all that alone?

p.p.s. Added a link to the CR into the draft release note. Hope that's cool. Feel free to edit / remove if needed.

p.p.p.s. Crediting @andypost and @catch for reviews.

p.s. I updated the CR about the new deprecations in 10.4.x → .

Thanks, y'all! Pushed commits to address the review of the backport MR in #27:

1. Added deprecation to core/modules/update/src/Form/UpdateManagerInstall.php (both a @deprecated in the class comment itself, and a trigger_error() in the constructor.
2. Cherry picked 2 commits from the 11.x MR into the 11.0.x MR to address point 2 about keeping docs in sync.
3. Added deprecation to update_authorize_run_install() (and then fixed the syntax error I introduced 😅)

Should we backport the deprecations in core/lib/Drupal/Core/Updater/*, too? And change them to being deprecated in 10.4.0? Or leave all that alone?

Re: docs edits in #28: those might be better handled via the parent meta, no? There's already a lot of talk of docs that need updating in there. I guess we could open a specific issue to handle the docs only around this install UI as a child of this one. I defer to the release managers...

Anything else before this is ready?

Thanks again,
-Derek

Whoops. I never commented here when I linked this issue as one of the points for 🌱 [meta] Deprecate tarballs, because they are incompatible with Composer-managed dependencies, Automatic Updates, Project Browser, and release managers' health Active . What’s the status of this? Is it worth immediately resurrecting for 11.0.0 release? Possibly 10.4.x? Can we in fact convert this manual thing to a composer-based solution without needing all of auto updates workings and stable?

Thanks!
-Derek

P.s. I just pinged the parent meta about progress here. Probably worth reconsidering the backports here in light of that discussion.

Also, how would we deprecate a form and routes, if we did want to do that? 😅