TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given

This became more relevant in 10.3, which removes the country setting and the implicit empty string value being set there, resulting in this fatal error with basically all tests on 10.3 right now. See 📌 Remove country setting from the installer Fixed . This might still change, but it doesn't hurt to make this change. It's quite possible that the setting will be completely removed from core in the future.

The relevant core issue is actually 📌 Add validation constraints to system.date Fixed

Bumping priority because this will become a larger issue for new installs once 10.3 comes out I think.

nicxvan → changed the visibility of the branch 3173772-typeerror-argument-1 to hidden.

nicxvan → changed the visibility of the branch 3173772-typeerror-argument-1 to active.

nicxvan → changed the visibility of the branch 8.x-2.x to hidden.

I converted this to an MR, I think this might need some work though, shouldn't resolve return a Country no matter what?

I imagine we'd follow what Price does in .install and set a specific country as default if one is not set on the site.

I'm moving to needs review for that question.

Also should we just remove the three references entirely?

At least the local resolver, which uses the country resolver checks if there really is a return value.

Because it was optional before as well, the only change is the move from empty string which was bogus but not broken, to null.

So if there is a possibly to have a empty string, shouldn't we simply check if the country code isn't empty? And return NULL otherwise? Instead of having an instantiated country with an "" code?

That's what the code kinda does, I think the is_string() call is not necessary. The difference is just that an empty string kinda worked until now, while a null value doesn't work at all.

Found this thanks to @Berdir's comment at #3437325-42: Add validation constraints to system.date → .

Interesting:

[error]  TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given, called in /var/www/html/web/modules/contrib/commerce/src/Resolver/DefaultCountryResolver.php on line 35 in Drupal\commerce\Country->__construct() (line 23 of /var/www/html/web/modules/contrib/commerce/src/Country.php) #0 

👆 None of that code is in Drupal core: neither the Country class nor the DefaultCountryResolver class.

It seems that the problem here is that this module has built infrastructure that assumes a certain data shape ("strings only"), but it does not control the shape: Drupal core does.

Because of that (fine!) choice, this module's infrastructure should adhere to https://en.wikipedia.org/wiki/Robustness_principle: — in this case it probably should detect whether the data is anything else than a 2-character string, and if so, fall back to the empty string (which this infra has assumed to be the fallback value so far).

I'd be in favor of returning NULL instead of instantianting a country with an empty string...
The CountryResolverInterface has the following documentation.

 * Resolves the country.
   *
   * @return \Drupal\commerce\Country|null
   *   The country object, if resolved. Otherwise NULL, indicating that the next
   *   resolver in the chain should be called.
   */

In case of an empty string, a country would be considered as resolved, thus the next resolver in the chain isn't going to be invoked, which sounds like a bug to me.
So IMO we should simply check if it's empty, or check if the config value is a 2 characters string.

@jsacksick? Yes, that's exactly what the MR does? It additionally also checks for it to be a string to be extra careful, just in case that config is something else, but an empty string will skip the check and return NULL

@Berdir: No, I don't want a country to be resolved when the string is empty "".

if ($country_code && is_string($country_code))
An empty string will pass the condition?

Merge the MR, thanks!

Thanks! I also just want to make it clear, this probably wants to make it into a recommended release before Drupal 10.3 drops since that introduces the upstream change.

It seems we have quite some time ahead of us (Drupal 10.3 will be released on the week of June 17, 2024).
But in any case, a new Commerce release is already a bit overdue, I know several contrib patches are depending on code that is already in dev but not yet part of a release.

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