Add validation constraints to system.date

narendraR → changed the visibility of the branch 3437325-add-validation-constraints to hidden.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Interesting failures:

Drupal\FunctionalTests\Installer\InstallerSiteConfigProfileTest::testInstaller
Behat\Mink\Exception\ExpectationException: The field "date_default_timezone" value is "Africa/Abidjan", but "America/Los_Angeles" expected.

and

Drupal\FunctionalTests\Installer\InstallerTest::testInstaller
Failed asserting that null matches expected 'Australia/Sydney'.

This is looking very close though! :)

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

No, bot, it does not need work.

Only one tiny comment, then I don't really see a problem here.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

📌 Remove country setting from the installer Fixed landed today! It conflicts with this MR.

I don't see how this could land without at least an update path — fortunately it's a trivial one! I suspect we'll also need explicit tests for \Drupal\system\Form\RegionalForm.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Good progress!

In the final stretch to the finish line now :)

Problems I spotted in review:

1. validation of timezone.user.default is misleading — easily fixed 👍
2. form test coverage is giving a false sense of security — it should be a functional test, not a kernel test

P.S.: please push 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work forward so that we can land ✨ [PP-1] Enum cases stored in config Postponed — that'd make validation of timezone.user.default in this issue simpler/more maintainable/clearer. It'd also help many other (config) validation issues!

Some minor points, but this generally looks pretty good to me.

Ship it!

\Drupal\system\Form\RegionalForm::getEditableConfigNames can be removed and the form can use \Drupal\Core\Form\RedundantEditableConfigNamesTrait now.

3 nits, one of which seems indisputably an improvement, so applied that. The other 2 I'll leave to the core committer reviewing this.

Other than that: looks great! 😊

I don't think we should be suing static methods and \Drupal::service - see MR comments.

Also do we have any BC concerns about moving from empty string to NULLs?

https://git.drupalcode.org/project/drupal/-/merge_requests/7271/diffs?co... → 🤩

Too bad you reverted and went with a custom constraint in the end. I think what you did originally would've been more broadly applicable.

OTOH … what you've done now is declarative and is easier to interpret on the client side! This is now essentially a validator for https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2. It's similar to Symfony's \Symfony\Component\Validator\Constraints\Country, but that depends on symfony/intl, which Drupal core does not use.

Conclusion: this change is 👍😄

P.S.: I think a change record to allow others to use the new CountryCode validation constraint would be great 😊 But that can easily happen post-commit.

Too bad you reverted and went with a custom constraint in the end

Yeah, I was disappointed I had to revert also. But I realized that there was a very subtle bug in my original approach, just waiting to give somebody a splitting headache. The problem is that getAllCountryCodes() is not part of CountryManagerInterface. So the validation constraint was coupled to a non-interface service method.

If someone (or some module) swapped out core's CountryManager for a different implementation of CountryManagerInterface -- one without the validation method -- they would have run into some very obscure, hard-to-trace errors.

In light of that, I decided it was best to go with a custom constraint that wraps specifically around CountryManagerInterface::getList(), which is an interface method and therefore won't break if the implementation is swapped out.

I agree that making Choice aware of the callable resolver is likely to be useful in the future, and as soon as we have a concrete case for it, I'll happily restore it. :)

I yhtink the test is in the wrong location. I suggested a kernel test but a unit test would work just as well i think

#33 is a great point — it made swapping out the default implementation impossible indeed! 👍

The unit test @alexpott requested already passed, so re-RTBC'ing even before CI is completely done.

P.S.: change record? 🤞

Done. https://www.drupal.org/node/3441838 →

Committed and pushed 8cd02c7f00 to 11.x and 338d4d046a to 10.3.x. Thanks!

That was a deliberate choice on my part. If I used real country codes, there's an infinitesimally slim chance (maybe some accidental bad refactoring in the future) that we could be calling the real country manager, instead of the mock. Using fake country codes proves, unambiguously, that the mock was called.

Wow, excellent call, @phenaproxima! 👏

quietone → closed merge request !7271

This breaks commerce hard, because it expects that country is a string, so this change results in a fatal error there atm. See 🐛 TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given RTBC . While it can easily be fixed there and the change makes sense anyway, instead of returning a Country object with an empty country string, this is still a pretty hard BC break.

#42: I don't think that's entirely accurate; that module does something rather special: #3173772-15: TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given → — it uses core config as if it was its own config (with schema it controls). I do not think that is a widespread practice?

If anything, after this (somewhat painful — although the changes in the module seem trivial) transition, any module doing something like this will have stronger guarantees after the value is being strictly validated: they'll know much more precisely what data to expect.

The combined usage with the strict type type is special, but using that config is not.

From your comment in the other issue:
> IOW: this is all caused by this module using core config as if it was this module's config.

That's not a thing. Drupal has the concept of a default country, and reading this config file is the only way to access it. There is no rule that you're not allowed to read config of other modules.

Using config like that is common. See https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22get%2...

https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → is IMHO a bit confusing, as it then only lists composer related config, but I understand that as changing/removing config structure is covered by our BC policy.

Drupal has the concept of a default country, and reading this config file is the only way to access it.

Hm, good point! 😬

What path forward do you propose?

> What path forward do you propose?

I don't know. The commerce issue has been fixed and will be available in a release soon. It's now in the alpha release, so I guess reverting this would just cause a bigger confusion. Might hit people trying out the alpha release, but nothing can be done about that at this point I guess except getting out a new commerce release out asap.

Automatically closed - issue fixed for 2 weeks with no activity.