Locales are required, but do not save locales hyphenated langcodes correctly

Notes

• when rendering the form and setting default locale codes for each langauge
• BazaarvoiceService::getBazaarvoiceLocaleCode transforms a langcode of fr-ca to fr_ca here
• BazaarvoiceService::isValidLocaleCode() returns false when validating fr_ca

patch does not apply when using composer; only include relevant changes and I'll try again.

Thanks for the patch santoine!

I took some time to review methods getBazaarvoiceLocaleCode(), and its related methods: isValidLocaleCode, getLocaleCode, findDefaultLocale and defaultLocaleCodes().

I've also reviewed the BV documentation on locales and supported locales.

The Bazaarvoice platform support multiple locales which are identified using the language_COUNTRY format.

BV supported locales. are not 1:1 with the list of the modules default codes seen here, e.g af_NA (first default code) is not a supported BV code.

I don't think it makes much sense to manage default locales in code as BV may change that. I created #3143372: Replace default codes with help text and link to supported BV locales.

Regarding the original reported issue and getBazaarvoiceLocaleCode(), I don't see a need to transform or validate the langcode at all in order to fetch the mapped locale code, do you?

I think the $locale_code = $langcode; confuses things greatly b/c while it makes sense to validate BV locale codes for its expected language_COUNTRY format. However, when getBazaarvoiceLocaleCode validates it's validating the langcode, not BV locale code.

Currently,

  public function getBazaarvoiceLocaleCode($langcode) {
    $locale_code = $langcode;
    // Do some basic transformation.
    $locale_code = preg_replace('/[^a-zA-Z]+/', '_', trim($locale_code));

    if (!$this->isValidLocaleCode($locale_code)) {
      if ($code = $this->getLocaleCode($locale_code)) {
        $locale_code = $code;
      } else {
        $locale_code = $this->findDefaultLocale($locale_code);
      }
    }
    return $locale_code;
  }

Recommended change to preserve the possibility of this return FALSE as previously observed.

  /**
   * {@inheritdoc}
   */
  public function getBazaarvoiceLocaleCode($langcode) {
    $locale_code = FALSE;

    if ($code = $this->getLocaleCode($locale_code)) {
      $locale_code = $code;
    }

    return $locale_code;
  }

However, now looking at this, getBazaarvoiceLocaleCode() and getLocaleCode() can be the same and thus getBazaarvoiceLocaleCode() and remove getLocaleCode(); it was only being used by getBazaarvoiceLocaleCode().

  /**
   * {@inheritdoc}
   */
  public function getBazaarvoiceLocaleCode($langcode) {
    $locale_code = FALSE;
    if ($locale_codes = $this->getLocaleCodes()) {
      if (isset($locale_codes[$langcode])) {
        $locale_code = $locale_codes[$langcode];
      }
    }
    return $locale_code;
  }

Looks good!

Looks like setLocaleCodes() is used in AdminSettingsForm::submitForm(), so we'll need to keep this method, which sets the mapped langcode to locale to bazaarvoice.locales.

+++ b/src/Service/BazaarvoiceService.php
@@ -28,35 +28,19 @@ class BazaarvoiceService implements BazaarvoiceServiceInterface {
-  public function setLocaleCodes($locales) {