Remove country setting from the installer

Alternatively, if we are to keep this setting, then we should make it useful - for example, it should set up date formats correctly for the country that was selected.

I'm curious about this after finding your comment on [1933614]

I'm going to do a naive attempt for removing it from the form to see which tests fail. I'm also not sure if the Dateformatter can be done separately since it looks for this setting.

+1 it could be moved to contrib module

So tests pass with this removed. I think this depends on 3439440 📌 Remove country support from DateFormatter Active
Since 3439440 uses this setting and that issue is set to remove it.

The only other place using this in core other than the mentioned issue is the regional form that you can change the setting.

The date formatter reads the value but never uses it, so this can be done first if needed.

We can remove CountryManager from the form here, it's not used any more.

This might be a dumb question, but I can't actually remove it, I need to follow this right? https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... →

Yup we should try to provide backwards compatibility in case anyone has extended the form.

I'm actually not sure of the best way to handle this since the next property is a new item with a deprecation warning as well.

Current docblock

/**
   * Constructs a new SiteConfigureForm.
   *
   * @param string $root
   *   The app root.
   * @param string $site_path
   *   The site path.
   * @param \Drupal\user\UserStorageInterface $user_storage
   *   The user storage.
   * @param \Drupal\Core\Extension\ModuleInstallerInterface $module_installer
   *   The module installer.
   * @param \Drupal\Core\Locale\CountryManagerInterface $country_manager
   *   The country manager.
   * @param \Drupal\user\UserNameValidator|null $userNameValidator
   *   The user validator.
   */

Is the right thing to do is to add usernamevalidator as a type hint option on to $country_manager
and check to see if country manager is the country manager or username?

There can be confusion because there is a new deprecation from 3 days ago on username validation.

I made an attempt at deprecating it.

I think this approach depends on it getting in before the next release though since the constructor just changed last week.
If this does not get in we probably need to deprecate this differently and keep the optional username validator.

There was some discussion in slack between, chx, catch and myself about whether deprecations are necessary.

The conclusion is that it is more necessary for services to prevent contrib breakage, and it is attempted in most cases where possible.

Changing deprecations within a minor release is also fine as far as I can tell, so this should be good. I'll move to needs review if the tests still pass.

Also @berdir pointed out that the installer is explicitly not part of BC.

I was about to RTBC, but I noticed something fishy with the $deprecatedProperties definition. Added an MR suggestion about it.

Other than that, I think this is ready.

Thanks!
-Derek

I applied @dww's suggestion and also took a screenshot of the new install page.

I'm also going to write a change record since I assume it is needed for a change like this.

This is changing the UI so adding tag per
https://www.drupal.org/about/core/policies/core-change-policies/core-gat... → . This should also have before and after screenshots that a reviewer can find from the Issue Summary.

Ideally, this should also provide the issue where the use of the date formatting was removed.

I think this should have a usability review because not only is is changing the UI it is changing the a form in the install process. I am adding the tag but correct me if I am wrong.

• The changes in the MR are entirely within core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php and now look proper.
• The deprecation dance looks good to me.
• Pipeline is all green.
• Summary and title are clear and accurate.
• The CR is short and sweet, no complaints.
• There's even a screenshot of the installer after the change, although I don't think that was strictly necessary. 😅

Ooof, x-post with @quietone. Replying to #23:

I'm not sure what a UX team review would add here. This is a totally useless setting that has no impact on core behavior. Less is more. Removing things from the installer we don't need is a good thing. There's 0% chance that a UX review would say: "nah, we should keep asking something that core completely ignores during people's very first impression of Drupal". 😅

Meanwhile, adding a before screenshot, and embedding both in the summary.

I believe this is now RTBC.

Thanks,
-Derek

p.s. Re:

Ideally, this should also provide the issue where the use of the date formatting was removed.

See comment #8 and related issues. Int'l date formatting was removed at #2276183: Date intl support is broken, remove it → . Added that to the summary, too.

Cheers,
-Derek

@dww, thanks for updating the Issue Summary! I know that can be tedious.

I now see the original issue in the Issue Summary. Thanks! Yes, I did not read this entire issue for that information, I was relying on the Issue Summary.

I read the original issue and didn't find any specific mention of the country field in the installer. The closest I found was a sentence in the related change record that support might be added back at some future date. Removing the field does not change that it could be added back. So, that resolves any concern that I had.

Thanks again.

I changed the deprecation notice to be more accurate so it's not confusing since it is still required.

with the $userNameValidator argument as CountryManagerInterface is deprecated in drupal:10.3.0 and will be required in drupal:11.0.0

is now:

with the $userNameValidator argument as CountryManagerInterface is deprecated in drupal:10.3.0 and must be UserNameValidator in drupal:11.0.0

Fixing summary content I accidentally changed.

nod_ → closed merge request !7368

Committed 68365af and pushed to 11.x. Thanks!

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

Tagging for release not inclusion.

@nicxvan could you expand on why you think this change should be called out? The setting doesn't do anything. Why would it be disruptive for it to not be set on a new install?

@pamneela good point actually. Existing sites already have the setting. New installs of core don't need it. And for people that need it after installing they have the change record. I've untagged.

Sorry for the confusion.

Thanks for confirming.

I think this issue should have considered contrib. There are usages there, especially in commerce sites. Especially in commerce - including code that assumes it is set. See http://codcontrib.hank.vps-private.net/search?text=country.default&filen... - especially the default country resolver.

It was considered, we opened an issue specifically in Commerce to discuss it beforehand and they had only added it in a couple months before so were not opposed to removing it again.

I also built a list and opened issues in project where it could be an issue: https://www.drupal.org/project/address/issues/3444987 📌 \Drupal::config('system.date')->get('country.default') now has validation and can now be null Active

I then also searched contrib, almost all instances were distributions that contained core committed. I then created issues for contrib and highlighted the change, almost all instances used it as a fallback so nothing should have broken.