- Issue created by @mvdve
The last submitted patch, invalid_locale.patch, failed testing. View results →
- Status changed to Needs work
about 2 years ago 3:49pm 14 February 2023 - 🇳🇱Netherlands ndf Amsterdam
With the example: drupal installation in German, visitor is in the Netherlands,
what would be the correct langcode for Mollie?Is it a correct assumption that the langcode is used by mollie to display their dialogs in the visitor language?
If that's the case indeed the drupal interface-language should be used.NB we have a test that now fails
\Drupal\Tests\commerce_mollie\Functional\MolliePaymentOffsiteFormTest::testPaymentTransactionData
which assumes the following:$language = ConfigurableLanguage::createFromLangcode('fr'); $language->save(); // ... doing a transaction ... // The commerce locale resolver tacks the current language to the store // country. $this->assertEquals('fr_US', $last_transaction['locale']);
So this was actually all the time wrong and it should be:
$this->assertEquals('fr_FR', $last_transaction['locale']);
Is that correct? - 🇳🇱Netherlands mvdve
Yes that's correct, fr_US is not a valid locale code as French is not an official language of the US.
The second issue is that Mollie supports a selection of ISO 15897 codes, see locale in the docs and indeed, the locale variable is used to set the payment interface in the correct language.
Because of the selection of locales on Mollie's end, I mapped the available locales to the drupal interface locale and otherwise Mollie can do it's own thing, not the most generic method but it prevents the request error.
Updated the patch with the correct locale code within the test.
- Status changed to Needs review
about 2 years ago 6:41pm 14 February 2023 - Status changed to RTBC
about 2 years ago 7:43am 15 February 2023 - 🇳🇱Netherlands ndf Amsterdam
mvdve, this looks good thanks.
One more question. What is the reason to use\Drupal::languageManager()
instead of\Drupal::service('commerce.current_locale')
The default locale resolver of commerce seems to do a very similar thing see
\Drupal\commerce\Resolver\DefaultLocaleResolver::resolve
public function resolve() { // The getCurrentLanguage() fallback is a workaround for core bug #2684873. $language = $this->languageManager->getConfigOverrideLanguage() ?: $this->languageManager->getCurrentLanguage(); $langcode = $language->getId(); $langcode_parts = explode('-', $langcode); if (count($langcode_parts) > 1 && strlen(end($langcode_parts)) == 2) { // The current language already has a country component (e.g. 'pt-br'), // it qualifies as a full locale. $locale = $langcode; } elseif ($country = $this->currentCountry->getCountry()) { // Assemble the locale using the resolved country. This can result // in non-existent combinations such as 'en-RS', it's up to the locale // consumers (e.g. the number format repository) to perform fallback. $locale = $langcode . '-' . $country; } else { // Worst case scenario, the country is unknown. $locale = $langcode; } return new Locale($locale); }
Lets add a comment that we intentionally not use that resolver.
I'll do that on commit.
- 🇳🇱Netherlands ndf Amsterdam
Another thought; the supported list on Mollie is:
Possible values: en_US en_GB nl_NL nl_BE fr_FR fr_BE de_DE de_AT de_CH es_ES ca_ES pt_PT it_IT nb_NO sv_SE fi_FI da_DK is_IS hu_HU pl_PL lv_LV lt_LTWe now not have en_GB nl_BE fr_BE de_AT (and probably more)
What if we keep the commerce.current_locale logic for the making the locale code,
but if it's not in Mollie's supported list we remove it.
So when the lang-code is not in the valid list, we don't send$transaction_data['locale']
.
Then the following happens: When this parameter is omitted, the browser language will be used instead if supported by the payment method. - 🇳🇱Netherlands ndf Amsterdam
@mvdve what do you think from my comment above?
- 🇳🇱Netherlands mvdve
I replaced the commerce.current_locale service for the language_manager service because of the invalid locale codes.
In my case this was invoked:
elseif ($country = $this->currentCountry->getCountry()) { // Assemble the locale using the resolved country. This can result // in non-existent combinations such as 'en-RS', it's up to the locale // consumers (e.g. the number format repository) to perform fallback. $locale = $langcode . '-' . $country; }
Which returns invalid codes as described in the comments. Maybe i am missing something but i think this service should only return valid codes or none at all. Using the commerce.current_locale would result in a lot of false negative results with the mapping to the Mollie locales. For example current country is NL, site locale is DE, this would result in de_NL which is invalid. The language_manager service will return the site locale DE which can be mapped to one of the DE Mollie locales.
I don't know if Drupal has functionality for country specific language versions, so de_DE and de_AT for example. I was lazy and picked one country of each language to keep the switch block short. I agree that it would be best to map against the complete locale code, but i don't have these in my Drupal sites. Just double character locale codes.
- Status changed to Fixed
about 2 years ago 3:37pm 28 February 2023 - 🇳🇱Netherlands ndf Amsterdam
Thanks mvdve, I just committed the patch as is.
After the patch we have this updated test:
@@ -167,7 +167,7 @@ class MolliePaymentOffsiteFormTest extends CommerceBrowserTestBase { $this->assertEquals('2', $last_transaction['metadata']['order_id']); // The commerce locale resolver tacks the current language to the store // country. - $this->assertEquals('fr_US', $last_transaction['locale']); + $this->assertEquals('fr_FR', $last_transaction['locale']); $this->assertEquals($this->store->getName() . ' order 2', $last_transaction['description']->__toString()); }
where indeed
fr_FR
seems a lot better as default thenfr_US
.
This happens when a customer buys a product on the following url:/fr/product/1
- 🇳🇱Netherlands ndf Amsterdam
It's not in the
1.10
release of today, but only in the develop branch.
So that people have time to test this on their multi-language websites. - Status changed to Fixed
about 2 years ago 2:19pm 19 March 2023 Automatically closed - issue fixed for 2 weeks with no activity.