🇺🇸United States @dww

@Jaypan: Great, thanks!

@sickness29: Welcome aboard!

Cheers,
-Derek

Hi! Thanks for the offer. Apologies for the delays. I’ve been incredibly slammed in many areas of my life for the last many months.

Given my experience of your help and good work on datetime_extras, I would gladly grant you access. Alas, I don’t have permissions to do so myself. Only Jaypan has that power at the moment.

Assigning to them and bumping status.

If nothing else happens in a few weeks, maybe we can convert this into a request to make both of us full maintainers…

p.s. I tried triggering the test-only GitLab CI job, but it's all confused by the #2640994 changes. Probably test-only via GitLab won't tell us much until that issue lands. However, locally, if I revert the fix and just run the new test, I see:

There was 1 failure:

1) Drupal\Tests\views\Kernel\Handler\ArgumentSummaryTest::testArgumentSummary
Failed asserting that '1 (4) 2 (2)' contains "phk5vh2z (4)".

Which is exactly what we'd expect. The summary is only showing term IDs and the node counts, not the term labels...

I searched (a lot), and didn't find any test coverage of argument summaries. So I wrote a Kernel test for this on a flight earlier today. The MR still has a ton of changes since this needs 🐛 Fix label token replacement for views entity reference arguments RTBC to land. But I squashed that into a single commit in this issue fork, so it'll be easy to rebase that out of here once that issue lands. If anyone wants to review, the actual changes are just 3 commits:

https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...
https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...
https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...

larowlan → credited dww → .

Wow, what a colossal PITA. 😆 Guess it's good I jumped through all those hoops, since:

1. There were indeed some test views with stale argument plugin definitions that needed to be manually fixed, including the view for testing this new functionality. 😂
2. EntityArgumentUpdateTest was making some slightly bogus assertions.

However, I had to completely rewrite ViewsConfigUpdater::processEntityArgumentUpdate() to operate on entire View entities, instead of using all the per-handler plumbing we've got. To my great dismay, if you copy the existing patterns, ViewsConfigUpdater process functions make a bunch of changes to handler config, then try to save the view, but that instantiates another ViewsConfigUpdater object (this time, with deprecations enabled) to do all the work again. 🤯 So apparently, The Right Way™️ to update a view with all this plumbing is that your process function has to directly update the $view (which isn't even normally passed in to your process function if you think you're just dealing with handlers), not just fix the $handler. 🤦‍♂️

I guess I'm now in the exclusive club of ~5 people on Earth who understand how this class works. 😬 😂

ANYWAY, we're back to green pipelines on 11.x and 10.3.x. I've given up on the 10.2.x MR at this point. It works great for the project where I needed this fixed, all the additional hassles are irrelevant to that site, and this is clearly not getting backported. I just hope this can still land in 10.3.x!

Thanks,
-Derek

I got a bit more clarity from @catch on what's expected here. I pushed some commits to the 11.x and 10.3.x MRs to trigger deprecations when ViewsConfigUpdater::processEntityArgumentUpdate() changes a view outside of post_update. I'm not sure on the wording of the deprecation message, that could use another pair of eyes. Also unclear if we "need" tests that this deprecation plumbing works as expected. I sure hope that's not my responsibility here. 😅

I'll keep an eye on the pipelines to make sure I didn't break anything, but I hope this is now actually RTBC.

Groovy, thanks. Ship it! 🎉

You're probably hitting this core bug: 🐛 Fix label token replacement for views entity reference arguments RTBC . Sadly, this project can't help you with that. It's a bug in the Argument plugin itself, and that can't be solved by an ArgumentValidator plugin like the one provided here.

I asked in #bugsmash for help on this. @catch pointed me to 📌 ckeditor5 and editor module tests rely on hook_editor_presave() bc layers Active , but that issue has nothing to do with ViewsConfigUpdater. I still don't exactly understand what's being asked of me.

Views is doing this:

function views_view_presave(ViewEntityInterface $view) {
  /** @var \Drupal\views\ViewsConfigUpdater $config_updater */
  $config_updater = \Drupal::classResolver(ViewsConfigUpdater::class);
  $config_updater->updateAll($view);
}

@alexpott, are you proposing we should change the API for ViewsConfigUpdater::updateAll() to take a 2nd argument to determine if we're being called from presave or post_update? Or do you propose that views_view_presave() checks the return value from ViewsConfigUpdater::updateAll() and if it ever gets back TRUE it should always trigger a deprecation?

Can we pretty please move all such complications to a dedicated follow-up, and fix this before the 10.3.0-beta1 ships? I still fail to see why this poor bug needs to be the one to deal with all that additional functionality.

Tentatively moving back to RTBC to get more committer eyes on this.

Thanks,
-Derek

Probably it’ll be 11 before this happens, but at least bumping to 10 for now

Looks good to me, though we probably want @phenaproxima to formally accept in here before this is committed.

In very mild local testing, the change in the MR seems to work. This is the first time I'm looking closely at payment gateways at all, much less the gory details of Stripe. Would love some reviews / pointers on if this is legit. 😂

dww → created an issue.

FYI, the real bug is at ✨ [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter Needs work

Rebased the 11.x MR to resolve conflicts.

@alexpott: Not sure what you mean with this:

I think we need to trigger deprecations from the ViewConfigUpdater when it is fixing a view during preSave that's not done as part of the post update. This way modules will know that they have to update their shipped content.

Sounds like scope creep to me. I don't want to be rewriting ViewConfigUpdater to add new functionality like this to try to fix this bug. Can we address that in a follow-up?

Thanks,
-Derek

I opened an MR thread to document how the diff is removing an exception. I'm a little sad to change the title at all, but how's this?

Hate to ask, but do we need a change record for this?

I landed here chasing various things. For how long this issue has been open, how many comments, patches and effort went into this, I was amazed / shocked / delighted / heartbroken that the diffstat on the changes tab in the MR is: +7 / - 3. 😂 Thrilled to see it's actually RTBC at this point, and hopefully going to land soon. +1 to the RTBC. The code changes are great. The test change is that we now get to check for better outcomes to a common DX problem. 🚀 it!

Thanks,
-Derek

p.s. Starting to save credit. Could use a closer look before commit.

I tried this locally:

-  public function summaryName($data) {
+  public function summaryName(array $data): string|\Stringable|NULL {

Then this:

./vendor/bin/phpunit --debug -c core/phpunit.xml core/modules/views/tests/src/Functional/Plugin/EntityArgumentTest.php

Which resulted in this:

There was 1 failure:

1) Drupal\Tests\views\Functional\Plugin\EntityArgumentTest::testArgumentTitle
Test was run in child process and ended unexpectedly

Reverting the type changes to summaryName() and that test is passing fine. So either I botched it somehow, or this just won't work until we add types to Views plugins (or at least to Argument plugins). /shrug

Thanks for taking a look!

Re types: I don’t believe so, since this is implementing a parent method that doesn’t have types and that’d be an api change. I believe. 😂 I’m honestly not sure what our policy is on that.

longwave → credited dww → .

Looking at the rest of core/modules/field_ui/tests/src/Functional/ManageDisplayTest.php makes me cringe. Wow that's a lot of separate (tiny) test methods. 😢 Obviously out of scope to refactor all of this, but probably worth a follow-up.

That said, I'm not sure what I'd suggest for this issue, so maybe adding a new test method is fine. Restoring NR, but it smells a little fishy to me. 😅

Love this. Added a note about test efficiency to the MR. I don't think we need a whole new test (and therefore, complete install of Drupal) for 2 assertions. 😅 Let's move these into an existing test to save time / CO2 / DA $.

Otherwise, seems like a win to me, and probably very close to RTBC.

Thanks!
-Derek

Thanks for the reviews and RTBC! I pushed the bool return type to ViewsConfigUpdater::needsEntityArgumentUpdate() to both 10.3.x and 10.2.x MRs. Thanks for spotting that.

📌 Add test coverage for 404 on throbber-active.gif and friends Active

dww → created an issue.

Whoops, typos / incomplete title

: 📌 Nove MigrateUpgradeExecuteTestBase expectedLoggedErrors property to Active

dww → created an issue.

Do we need tests for this? I put that as the remaining task in the summary.

Adding STR. I tried testing locally and at first failed since Claro doesn't have the bug. You have to turn off Claro to see it in action.

I know this is tagged novice, which I am not 😂, but @catch pinged in #bugsmash about this since it's time sensitive. So I pushed fixes (I think) and opened an MR. My local can't run 11.x right now, but I'll see if this applies to 10.3.x and try to test it there.

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

Note to reviewers: Until #2640994 lands, the "Changes" tab in the MR is a ton of stuff you don't need to look at. This is the only change being proposed here:

https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...

Thanks,
-Derek

This is technically still blocked on 🐛 Fix label token replacement for views entity reference arguments RTBC . I pushed a single commit with all the changes from that issue into this issue fork, then another commit to actually fix the bug here. It'll be trivial to rebase this and remove the 1st commit once #2640994 lands. But I wanted to get this moving in the hopes we can fix both in time for 10.3.0.

The 11.x branch needed a rebase (with some fairly trivial conflicts) due to 📌 Removed deprecated code in taxonomy module Fixed

Off topic from this issue, but you should never point composer at a GitLab MR. Anyone could push broken or malicious code to the MR but the path doesn’t change and composer will happily try to apply the now broken or vulnerable code to your site.

Download the patch. Store it locally. Tell composer to apply that. I’ve got a clean checkout for core development. Apply it there. Make a new patch. Put that in your composer.

Re #5: Thanks for putting #3 into the summary.

Where does 'media' come from in "task(media):" ?

The spec:

<type>[optional scope]: <description>

My interpretation from #3:

The optional scope would map to an issue's Component. That's a nice touch, but I'm not as set on including that as the other aspects.

Re: #6: Maybe it's only because I've been doing this way in Drupal and every other kind of thing I code for decades, but having the "issue" ID prominent in the summaries is essential. There are lots of places in Git (CLI or otherwise) where *all* you see is the 1-line summary. I'd hate to not know the unique ID for the issue when scanning those.

Love it. I do stuff like this a lot in tests. I basically can't stand to see data providers in Functional tests, and heaven-forbid in FunctionalJavaScript.

The results in #8 are mind blowing, given the small size of the diff. 8:11 to 0:24 is more than a 20X speedup. 😂

Ship it!

Thanks,
-Derek

I'm new to these tests. Took me a moment to understand why we're moving assertLogError() outside the check for if we've written anything to the logs. But then with grep and looking at more context outside of the MR diff, I see that that function relies on $this->expectedLoggedErrors, which is only set in the two functions where assertLogError() is now found.

At the risk of slowing down this important fix, I have to ask: should that method really be something like:

assertNumberOfLoggedErrors(int $expected): void

Why bother with the expectedLoggedErrors property at all? It's only set exactly twice with hard-coded ints each time:

./core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php:    $this->expectedLoggedErrors = 27;
./core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php:    $this->expectedLoggedErrors = 39;

We never increment or anything. It's basically just a constant. Why not pass it directly where we need it?

That said, I'm going to RTBC this, since my refactoring concern doesn't need to hold this up. If y'all agree the above would be cleaner, but don't want to do it here, it'd be a novice follow-up.

Thanks!
-Derek

Pipelines are all green again, except the functional JS random fail in the 10.2.x MR mentioned above. I doubt the RMs are actually going to backport this to 10.2.x, and that MR is mostly for folks to patch their own live sites with until 10.3.0 is out and they can upgrade to it...

As usual, this fell off my radar. Tagging to be smashed. I can't do a full subsys maintainer review anytime soon, but trying to remind myself to get back to this ASAP.

Ahh, thanks. The 11.x MR was failing due to 📌 Remove the 9.4 database dump fixtures from 11.x Active . 😅 Pushed a commit to use the 10.3.0 fixture instead of 9.4.0. That works locally in the 10.3.x MR. My local can't run 11.x ATM. 😂 Also rebased both MRs to latest 11.x and 10.3.x core. We should be all green again. 🤞

Not sure what you mean. The ones I opened are very clean. 😂 I don’t have edit access to 7091, so the title and description are lacking, but does that matter? The summary explains what all 3 are for.

I needed this, and it was quite simple, so I opened an MR for it. There's no .gitlab-ci.yml for this project, so there's no pipeline, but it works, I promise. 😂

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

Opened 🐛 [PP-1] Use labels in Views argument summaries for entity references Active to help resolve the last of the threads, keep the scope smaller in here, and finish the job that was started at #2912332: Allow entity reference views arguments to display the entity label in titles and summaries → .

I believe this is RTBC, but obviously someone else will have to say so at this point. 😂

Thanks!
-Derek

dww → created an issue.

This is now being done in 🐛 Fix label token replacement for views entity reference arguments RTBC , which is almost ready to commit. 🎉

Thanks for the reviews! Crediting kim.pepper and Lendude.

Lendude asked about the difference between EntityArgument vs. EntityReferenceArgument. I wasn't sure. 😂 It was introduced at comment #120. I tried to remove the distinction and make everything use EntityArgument, but then Kernel/ViewExecutableTest::testArgumentValidatorValueOverride() started failing (see pipeline job 1377453):

1) Drupal\Tests\views\Kernel\ViewExecutableTest::testArgumentValidatorValueOverride
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    '{{ arguments.uid }}' => '0I[">&1Z'
+    '{{ arguments.uid }}' => ''
     '{{ raw_arguments.uid }}' => '1'
 )

The test view argument definition ( see core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_dependency.yml) seems weird:

        uid:
          id: uid
          table: node_field_revision
          field: uid
          relationship: none
...
          entity_type: node
          entity_field: uid
          plugin_id: numeric

Why entity_type: node if this is pointing to a user? I don't fully understand the use of entity_type in existing definitions...

So maybe it's safer (if a little clumsy) as two separate plugins. Although should one extend the other? Or perhaps we could use one plugin, but make sure it uses target_entity_type_id not entity_type?

For now, reverted the commits to unify the two, and instead pushed docs for both. All the relevant tests are passing locally again. The bot agrees, phew.

Meanwhile, per @catch in #d11readiness the 11.x branch is really 11.x now. We definitely need the separate MRs here. I cleaned up the 11.x MR to remove the deprecation tests and BC layer from EntityArgument. All that only lives in !7571 now. The 10.2.x MR (!7572) has the silent BC layer.

dww → created an issue.

Hrm, 🐛 [random test failure] Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest::testWidgetViews random fail Fixed seems to keep happening, or a new problem has arisen, or something. But WidgetViewsTest is randomly failing a lot right now, both on the bot, and locally, with or without this MR branch checked out. 😢

• https://git.drupalcode.org/issue/drupal-2640994/-/pipelines/150738 (for MR !7091 / 11.x) is currently green (other than the validateable config stuff).
• https://git.drupalcode.org/issue/drupal-2640994/-/pipelines/150753 (for MR !7571 / 10.3.x) is similarly green/yellow.
• https://git.drupalcode.org/issue/drupal-2640994/-/pipelines/150748 (for MR !7572 / 10.2.x) has previously been green, but is currently red. I've tried re-running that 1 job (FunctionalJS 2/2) But I don't want to waste too much bot time on this.

Hopefully that doesn't turn into a huge time-sink.

Meanwhile, I opened 1 thread (in 7091) about the only thing left in this MR that I'm concerned about. Feedback and reviews most welcome. I think we're darn close to RTBC now. 😅 Phew!

Thanks,
-Derek

I don't know what the bot is doing, but it's confused, so turning it off from this issue.

I realized we don't want to remove ViewsArgumentDeprecationTest, we want to fix it. 😅 I moved the deprecation dance to EntityArgument, so it now supports all the old legacy ways it might be instantiated / used since it's replacing all the other core classes. Updated ViewsArgumentDeprecationTest to trigger all the new deprecations and check for the right ones. I think we should edit the CR from 🐛 Return translated term name on views "Content: Has taxonomy term ID (with depth)" Fixed ( https://www.drupal.org/node/3427843 → ) to only talk about IndexTidDepth since the Taxonomy one is now gone.

The 10_2_x version safely handles all the legacy stuff but throws no deprecation notices.

Meanwhile I updated the CR here with the full explanation of all the new BC stuff: https://www.drupal.org/node/3441945 → Updated the API changes section of the summary.

Bah, the validatable config job is such a PITA. For a few weeks, it was failing to find a valid version of drush to install. Now it's complaining about this:

Installing Drupal
In install.core.inc line 966:
                                                                               
  Resolve all issues below to continue the installation. For help configuring  
   your database server, see the <a href="https://www.drupal.org/docs/install  
  ing-drupal">installation handbook</a>, or contact your hosting provider.<ul  
  ><li>The database server version <em class="placeholder">3.40.1</em> is les  
  s than the minimum required version <em class="placeholder">3.45</em>.</li>  
  </ul>                                                                        

I don't have time for this. 😂 I'm going to ignore that for now, and I hope everyone else does, too. Crossing that off from the summary.

So this is really ready for review now. Would love to see this finally smashed, ideally for 10.3.x. I suspect this is probably too potentially disruptive for a 10.2.x backport, but I'm going to keep that MR up to date since I definitely need this working on some 10.2.x sites.

Thanks!
-Derek

Via Slack, @catch pointed me to 📌 Set budgets rather than exact numbers of asset size assertions Downport . Rebased the 11.x and 10.3.x MRs so hopefully that should solve the FunctionalJS failures so far. 🤞

Meanwhile, I started a draft CR: https://www.drupal.org/node/3441945 →

Back to NR...

https://git.drupalcode.org/project/drupal/-/merge_requests/7239.diff works against 10.2.x as of right now. It doesn't apply cleanly directly with git apply, but it works with patch -p1 < 7239.diff. That's what I've been using. 😅

Bumping this to major since I believe this qualifies under:

Render one feature unusable with no workaround

https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

The entire feature of setting display titles based on argument values converted to labels is broken, and AFAICT there's no workaround.

Addressed #153 and converted the post_update to use ViewsConfigUpdater. However, EntityArgumentUpdateTest.php was still failing locally. Then determined that the fixture that test is using was based on a stale Views schema. I re-created the fixture view on a clean 10.2.x site, and now that test is passing locally. Pushed fixes to all branches. Let's see what the bot thinks now...

Pushed a few more commits to hopefully get more of the pipeline stages passing.

Meanwhile, one of the test failures is a new deprecation test added for 🐛 Return translated term name on views "Content: Has taxonomy term ID (with depth)" Fixed . Since we haven't shipped 10.3.x, can we remove that test entirely? This is probably a sign we need to be more careful about BC? Adding as related.

Embedding the screenshots from #149 into the summary, too, so we don't need those from anyone else. 😅

Thanks,
-Derek

I keep running into this bug. Due to attributes vs. annotations, this doesn't backport cleanly, so I opened new branches and MRs for 10.3.x and 10.2.x. Meanwhile, hiding all the old patches in favor of the 3 MRs.

I updated the summary. Hope that's clear enough for review.

Haven't looked into pipeline failures, so leaving at NW.

Let's get https://git.drupalcode.org/project/drupal/-/merge_requests/7091 passing ASAP. I can backport appropriate changes to the other branches / MRs. But they're here for now for other folks like me that need these fixed on 10.2.x sites.

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

Well, drat. There are legit test failures in both of these:

core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
core/modules/taxonomy/tests/src/Functional/TermTest.php

I'm out of time for today, but I hope to get back to this soon. 😅 If anyone else wants to run with it in the meanwhile, please do. 🙏

Thanks,
-Derek

Hiding all attached files and starting to save credits.

I just ran into this bug on a new project. I was amazed this was broken. I dug deep into the views code, only to find:

// @todo How do we apply argument validation?

buried in core/modules/views/src/Plugin/views/display/PathPluginBase.php.

Searched the d.o queue and landed here. Thankfully, patch #52 applies cleanly both to 11.x HEAD and my 10.2.5 site. Converted to an MR. Fixed the phpcs and phpstan errors. I haven't addressed #35 / #47, but wanted to get this back into a reviewable state.

Let's smash this bug!

Thanks,
-Derek

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

Clarifying the summary a bit

For the record, I’m not proposing the “standards” include PHPStorm instructions. I only added it as a related documentation change that we could / should do while fixing this.

-1 to removing it, seems like a useful overview of a site, especially ones you didn’t build yourself.

Maybe it should move to Structure somewhere?

I think for minimum disruption we would add a new perm, an update hook that adds it to roles with the existing one, etc.

I could imagine sites that want to give out ‘view site reports’ to roles for whom a complete list of all entity types and fields would be inappropriate, overwhelming, etc.

This page probably wants to live under Structure, not Reports, anyway, but that’s for another issue. 😅 It does seem like a useful report to get a sense of the IA of a site, especially if you didn’t build it yourself.

Not sure what to recommend here. 🙃 I completely defer to the committers for a way forward.

Meanwhile, trimming a lot of noise out of the summary here.

Looking more closely https://www.drupal.org/node/608152#naming → uses:

1. UpperCamel
2. lowerCamel
3. CamelCase

Ugh to #3. That should either be "UpperCamel", or we do:

1. UpperCamelCase
2. lowerCamelCase
3. UpperCamelCase

I don't want to slow this issue down, we can clean all this up later. But I believe we can tidy as we go like this. It's only the wording of the docs, not the underlying standard they describe.

Cheers,
-Derek

Sounds great. +1

I think including "Case" is helpful for understanding. The CR title seems more weird as "Use UpperCamel for enum and enum case values".

How about friendly amendment, we standardize on "UpperCamelCase" and update the one other reference to it as part of fixing this?

Cool thanks. Now that it’s ready, officially tagging for RM sign off. 😅

Thanks, @realityloop! Happy to be back, although I'm not sure how much time I'll have to work on this. But I'll do what I can, when I can. 😅

Meanwhile, congrats @acbramley! 🎉

1. Adding myself as another (enthusiastic!) supporter. I never understood why we required FQCNs for these.
2. Fixed the dates for supporters (h/t https://xkcd.com/1179 😆)
3. Started identifying the docs that need updating.

Tagging for a summary update since I'm out of time to start fleshing out the proposed changes. Also worth reviewing that I found all the spots we need to fix.

Thanks!
-Derek

We reviewed this in the coding standards meeting this week.

• Seems that the "problem" has mostly solved itself.
• Meanwhile, core is moving to replace all annotations with attributes.
• So there was broad agreement to close this one as outdated.

Thanks,
-Derek

We reviewed this in this week's coding standards meeting, and there was universal agreement that this shouldn't happen. 😂

@quietone: "I am thinking this is a won't fix."
@catch: "Yeah we have rolling PHP version requirements, as does PHP itself, won't fix is good."
@borisson_: "I agree, coding standards are a living thing, fixing them in time or per version of php seems like making it difficult. +1 to won't fix"
@kingdutch: "Agreed. Coding standards should be for supported features which should work across PHP versions. The only dependency we have on PHP version is for feature introduction (like with Enum) but in that case older versions can ignore the part of the standard because they're not using the feature."

Instead of commenting in Slack, I'll comment here: I also agree. 😆 Won't fix it is...

Thanks,
-Derek

📌 Update CacheTagOperation enum to use UpperCamelCase Fixed is there with an MR if we want to move quickly on that before the standard is formally adopted.

dww → created an issue.

Agreed with @quietone that we should use "UpperCamelCase" not "PascalCase" in our standards docs. Also took the liberty of removing two "should"'s in favor of a single "must" in the proposed standards text:

Enums follow the same conventions as classes → with the addition that for their cases the enums must use UpperCamelCase.
...

Also, edited the CR per the concerns in #40:

https://www.drupal.org/node/3439920/revisions/view/13524321/13525482 →

Updating remaining tasks. The CS committee was in broad agreement. Also, since there are already a few enums in core, and we really need to standardize before 10.3.0 ships, we want to "fast-track" this change. Tagging for needing announcement (although maybe we're going to jump over that formality in this case). 😂

LGTM. Thanks!

+1. I've worked with @acbramley a lot in the core queue, and they are careful, thoughtful, competent, and kind. I'm sure they'd do a great job helping keep this module moving forward.

Thanks!
-Derek

Agreed, thanks.

@bojanz: Thanks for the input! I didn't notice that it was added so recently. I'd be fine with reverting that commit. Then we're no longer referencing this setting at all, and core could go forth and remove it...

@nicxvan / @andypost: Looks like this is "won't fix", at least from our perspective here in address.module land. Do you want to just close this and open other issue(s) in other possible candidate projects? That seems cleaner than moving this one around.

No worries. That's (part of) what us subsys maintainers are here for. 😉

Meanwhile, this wasn't a bug. 😂

larowlan → credited dww → .

Adding credit for @nicxvan. There's no policy against credit for deprecation removals. @catch committed without participating here, so the credit UI didn't get updated as usual...

Thanks for opening this issue. But hrm, reasons I don't love this proposal:

1. This module has no global settings. It's "just" a set of render elements and field types. It seems like it'd be a little weird to add a whole configuration form for this one "global" setting.
2. There's already been a lot of trouble and bugs around default countries. 😅 But (I believe) it all works as expected now. If folks want a default country for a given address or country field, they can configure it at the field level.
3. Seems a little wonky how ./src/Element/Country.php is using \Drupal::config('system.date')->get('country.default');. It seems to only happen for Address or ZoneTerritory elements that are required and have no default value. I don't completely follow the need for it at all.

Personally, I'd rather just remove the references to it if core wants to remove this setting entirely, and not try to provide it ourselves. But I'll hand this over to @bojanz for input, since he understands these details better than I do...

Yeah, I saw 💬 Retire Ludwig? Postponed . But I wanted to open a new issue specifically to get eyes on that core meta from the maintainers here.

p.s. I just opened 📌 RFC for Ludwig maintainers: Core wants to require composer for everything Active to alert the Ludwig maintainers about this issue, since I figured those folks might care and want to chime in...