elixir-cldr / cldr

Elixir implementation of CLDR/ICU
Other
447 stars 33 forks source link

Gettext fallbacks in `plug_set_locale` #124

Closed erikreedstrom closed 5 years ago

erikreedstrom commented 5 years ago

This isn't a bug so much as a point of confusion. The following code is being invoked when the CLDR locale is more restrictive than the Gettext locale.

defp put_locale({:gettext, _}, %Cldr.LanguageTag{gettext_locale_name: nil} = locale, _options) do
  Logger.warn(
    "Locale #{inspect(locale.requested_locale_name)} does not have a known " <>
    "Gettext locale.  No Gettext locale has been set."
  )

  nil
end

If CLDR is defined as something like en-GB, and Gettext includes en, one would expect a match on the fallback.

Thoughts?

kipcole9 commented 5 years ago

Its a bug. The following code is specifically intended to fallback to an available Gettext locale. I haven't spotted the bug on a quick view so will investigate. Might be a simple as a - versus _ in comparisons. Thanks for the report, shouldn't be too much to fix.

  @spec gettext_locale_name(Cldr.LanguageTag.t(), Cldr.backend()) :: locale_name | nil
  defp gettext_locale_name(%LanguageTag{} = language_tag, backend) do
    language_tag
    |> first_match(&known_gettext_locale_name(&1, backend))
    |> locale_name_to_posix
  end

  @spec known_gettext_locale_name(locale_name(), Cldr.backend() | Cldr.Config.t()) ::
          locale_name() | false

  def known_gettext_locale_name(locale_name, backend) when is_atom(backend) do
    gettext_locales = backend.known_gettext_locale_names
    Enum.find(gettext_locales, &Kernel.==(&1, locale_name)) || false
  end

  # This clause is only called at compile time when we're
  # building a backend.  In normal use is should not be used.
  @doc false
  def known_gettext_locale_name(locale_name, config) when is_map(config) do
    gettext_locales = Cldr.Config.known_gettext_locale_names(config)
    Enum.find(gettext_locales, &Kernel.==(&1, locale_name)) || false
  end

  defp first_match(
         %LanguageTag{
           language: language,
           script: script,
           territory: territory,
           language_variant: variant
         },
         fun
       )
       when is_function(fun) do
    fun.(locale_name_from(language, script, territory, variant)) ||
      fun.(locale_name_from(language, nil, territory, variant)) ||
      fun.(locale_name_from(language, script, nil, variant)) ||
      fun.(locale_name_from(language, nil, nil, variant)) ||
      fun.(locale_name_from(language, script, territory, nil)) ||
      fun.(locale_name_from(language, nil, territory, nil)) ||
      fun.(locale_name_from(language, script, nil, nil)) ||
      fun.(locale_name_from(language, nil, nil, nil)) || nil
  end
kipcole9 commented 5 years ago

May I ask you to report what your plug statement looks like? I assume it includes a :gettext param?

And also is possible your backend module configuration? Does it have a :gettext option specified?

What l think is happening is that the :gettext_locale_name is being correctly resolved if the backend module has a :gettext configured. But it is not being correctly resolved if you only have :gettext defined in the plug configuration.

As I look at this now I think that having the :gettext option on the plug configuration is a holdover from the CLDR v1 days. I think gettext should only be configured on the backend module. But its late so my thinking may be a bit muddy.

erikreedstrom commented 5 years ago

Plug Statement

plug(
      Cldr.Plug.SetLocale,
      apps: [cldr: MyApp.Cldr, gettext: :all],
      from: [:query, :body, :session, :accept_language],
      param: "locale",
      cldr: MyApp.Cldr,
      gettext: MyApp.Gettext,
      # XXX: https://github.com/kipcole9/cldr/issues/106
      default: Cldr.Locale.new("en-GB", MyApp.Cldr) |> elem(1),
      session_key: "cldr_locale"
    )

Backend Config

config :my_app, MyApp.Cldr,
  locales: ~w(ca da en-GB es fi fr it nb nl pt ru sv),
  data_dir: "../../priv/cldr",
  gettext: MyApp.Gettext,
  providers: [Cldr.Number]

In this case, both have the :gettext configured.

Gettext has known locales of ['es', 'en']

Note: I think I can drop that default work around now πŸ˜‚

kipcole9 commented 5 years ago

Hmmm, I'm a little perplexed. Here's a passing test case with gettext configured on the backend module:

  test "that the gettext locale is a set when an ancestor is available" do
    opts = Cldr.Plug.AcceptLanguage.init(cldr_backend: TestBackend.Cldr)

    conn =
      :get
      |> conn("/")
      |> put_req_header("accept-language", "en-AU")
      |> Cldr.Plug.AcceptLanguage.call(opts)

    assert conn.private[:cldr_locale].gettext_locale_name == "en"
  end

And that backend module is defined as :

defmodule TestBackend.Cldr do
  use Cldr,
    default_locale: "en-001",
    locales: :all,
    gettext: TestGettext.Gettext,
    precompile_transliterations: [{:latn, :arab}, {:arab, :thai}, {:arab, :latn}]
end

And the gettext locales are:

iex(1)> Gettext.known_locales TestGettext.Gettext
["en_GB", "es", "en"]
kipcole9 commented 5 years ago

I'll adapt your configuration for testing and see what the issue is. Your objective is to set the global Gettext locale I assume, based upon the apps: [cldr: MyApp.Cldr, gettext: :all] configuration option?

erikreedstrom commented 5 years ago

Correct. Perhaps it's something deeper on our end? πŸ€” Will dig some more. Thanks for taking a look. πŸ™‡

kipcole9 commented 5 years ago

I'm still on it. I wonder if there is any difference if you remove the :gettext option from the plug statement? I don't expect so, but maybe ....

erikreedstrom commented 5 years ago

Reproducing locally seems to be causing us issues. :( We see it on the server on every request, but not on localhost. Grr.

I'll close this out since this is likely an issue with our setup. Thanks again.

UPDATE: If we can pinpoint, we'll re-open.

kipcole9 commented 5 years ago

Thats really curious. Anyway, I adapted your configuration as follows and I it seems ok:

  test "that a gettext locale is set again" do
    opts = Cldr.Plug.SetLocale.init(
          apps: [cldr: MyApp.Cldr, gettext: MyApp.Gettext],
          from: [:accept_language],
          param: "locale",
          default: "en-GB",
          session_key: "cldr_locale"
        )

    conn =
      :get
      |> conn("/")
      |> put_req_header("accept-language", "en-AU")
      |> Cldr.Plug.SetLocale.call(opts)

    assert conn.private[:cldr_locale].gettext_locale_name == "en"
  end

Note that this isn't exactly the same as yours since I am not setting the global Gettext locale, only the backend specific locale.

erikreedstrom commented 5 years ago

@kipcole9 As part of testing we uncovered that once we remove the :gettext from the config, the :all option for apps on gettext will fail.

== Compilation error in file lib/my_app/router.ex ==
** (ArgumentError) Gettext module :all is not known
    lib/cldr/plug/plug_set_locale.ex:358: Cldr.Plug.SetLocale.validate_gettext/2
    lib/cldr/plug/plug_set_locale.ex:117: Cldr.Plug.SetLocale.init/1
    (plug) lib/plug/builder.ex:302: Plug.Builder.init_module_plug/4
    (plug) lib/plug/builder.ex:286: anonymous fn/5 in Plug.Builder.compile/3
    (elixir) lib/enum.ex:1940: Enum."-reduce/3-lists^foldl/2-0-"/3
    (plug) lib/plug/builder.ex:284: Plug.Builder.compile/3
    lib/my_app/router.ex:6: (module)
    (stdlib) erl_eval.erl:677: :erl_eval.do_apply/6
    (elixir) lib/kernel/parallel_compiler.ex:208: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/6

Seems to be part of the validation check.

Still hunting the other thing.

kipcole9 commented 5 years ago

Agreed, confirm that is a bug and fixed on my dev machine. Will push an update soon.

erikreedstrom commented 5 years ago

@kipcole9 So it seems like it's a compilation issue maybe. Reopening the issue for tracking.

Local

MyApp.Cldr.known_gettext_locale_names()             
["en", "es"]

Deployed

MyApp.Cldr.known_gettext_locale_names()             
[]

We deploy with distillery, so perhaps there's something to that. πŸ€”

When running the config calls directly on the server, the locales are there.

iex(...@10.8.0.52)11> Cldr.Config.known_gettext_locale_names(MyApp.Cldr.__cldr__(:config))                                             
["en", "es"]
iex(...@10.8.0.52)12> MyApp.Cldr.known_gettext_locale_names                               
[]

It seems when the backend compiles, it doesn't see them yet: https://github.com/kipcole9/cldr/blob/40e2f05f2d53641a0b8e1964d757f4d628dea89d/lib/cldr/backend/cldr.ex#L101-L105

kipcole9 commented 5 years ago

I have pushed a commit that may fix the issue - or at least help diagnose it better. Any chance you could try from master?

One thing I did was remove "MyApp.Cldr" from the :dev environment. For my own convenience I was generating these in the :dev environment which runs the risk that if you define a module with the same name then there could be a name clash. At least that possibility is removed.

Part of the work has been to conform the parameters better in Cldr.Plug.SetLocale.init/1 so knowing wha the return of that function is will help diagnosis if the issue still persists.

:gettext is now definitely an optional key in the configuration - not a requirement.

kipcole9 commented 5 years ago

I would suggest that you don't call your backend modules MyApp - I use that only for documentation purposes. I think it would be more meaningful to call them something related to your actual app name.

erikreedstrom commented 5 years ago

πŸ˜‚πŸ˜‚πŸ˜‚ we don’t actually call it that. I scrub the name.

kipcole9 commented 5 years ago

Cool - I was hopeful that was true :-)

Have to think about the issue now with your additional comments. The plug validates that the :gettext module exists and raises if not so that would imply that it is compiled in advance.

erikreedstrom commented 5 years ago

Thanks for the quick turnaround. I’ll run a test in my morning (about 8 hrs from now).

kipcole9 commented 5 years ago

One way to test if its a compilation order issue it to require your gettext module before you define you backend module. For example:

require MyApp.Gettext

defmodule MyApp.Cldr do
  use Cldr, gettext: MyApp.Gettext,
  ...
end

It shouldn't be necessary because simply mentioning the module name in the defmodule would force the compiler to compile the backend.

The only other thing I can think of is that perhaps if you changed your gettext config (ie added a locale) perhaps the gettext backend wasn't recompiled?

kipcole9 commented 5 years ago

Published version 2.6.2 for your tests in the morning.

erikreedstrom commented 5 years ago

We built without cache and it didn't resolve. Neither did simply requiring the module.

We did find a work around, which is to actually invoke known_locales prior to the use directive. Discovered this worked while trying to see if Gettext saw them at compile time.

IO.puts(
  "Resolved the following Gettext locales #{
    inspect(Gettext.known_locales(MyApp.Gettext))
  }"
)

use Cldr, otp_app: :my_app, default: "en-GB"

It's ugly, but the problem is resolved.

Is there a lot of overhead to the known_locales call? I'm wondering if you think it's necessary to have it as a compile time resolution.

On the plus side, we can see in the Logs what Gettext we have available per backend πŸ˜‚

kipcole9 commented 5 years ago

Thats .... really irritating. And I'm having trouble creating a reproducible case. This is only exhibited as an issue if :prod if I understood your earlier report? Really appreciate you digging - and at least finding a workaround.

kipcole9 commented 5 years ago

Its not an especially heavy function so executing it a runtime is definitely an option which I'lll explore.

juanperi commented 5 years ago

hey, I will chime in here, as I think i'm facing this same issue, but from a different angle (if you want me to open a different one, please let me know) I've created an app where i can consistently get an error due to gettext locales not being detected at the right time: https://github.com/epilgrim/bug_app_ex_cldr

you could get the error with: mix deps.get && mix compile

......
==> cldr_test
Compiling 3 files (.ex)

10:14:17.845 [info]  Downloaded locale "es"
Generating CldrTest.Cldr for 5 locales named ["ca", "en", "en-001", "es", "root"] with a default locale named "en"

== Compilation error in file lib/cldr_test/cldr.ex ==
** (RuntimeError) Locale definition was not found for ca
    (ex_cldr) lib/cldr/config/config.ex:1227: Cldr.Config.do_get_locale/2
    (ex_cldr) lib/cldr/config/config.ex:1688: Cldr.Config.decimal_formats_for/2
    (elixir) lib/enum.ex:1327: Enum."-map/2-lists^map/1-0-"/2
    (ex_cldr) lib/cldr/config/config.ex:1677: Cldr.Config.decimal_format_list/1
    lib/number/formatter/decimal_formatter.ex:762: Cldr.Number.Formatter.Decimal.define_to_string/1
    lib/backend/decimal_formatter.ex:52: Cldr.Number.Backend.Decimal.Formatter.define_number_module/1
    lib/backend/compiler.ex:11: Cldr.Number.Backend.define_number_modules/1
    (ex_cldr) lib/cldr/config/config.ex:1932: anonymous fn/3 in Cldr.Config.define_provider_modules/1

The special thing in this case is that gettext contains one locale that is not configured in the list passed to Cldr https://github.com/epilgrim/bug_app_ex_cldr/blob/master/lib/cldr_test/cldr.ex#L5 This is a real production app problem, as we use the locale "ca" for debugging purposes (crowdin has a nice in_place translations functionality making use of a fake locale) This issue seems to be resolved if i uncomment the line https://github.com/epilgrim/bug_app_ex_cldr/blob/master/lib/cldr_test/cldr.ex#L2 which required the Gettext module before calling the use of Cldr

kipcole9 commented 5 years ago

Definitely a bug and a very helpful stack trace because I'm surprised to see that the locale "ca" isn't being downloaded so I'll start digging there. I'm still perplexed why require Gettext.Backend is necessary but I assume its to do with macro behaviour.

I will dig into this today - thanks very much for the reproducible test case.

kipcole9 commented 5 years ago

@erikreedstrom, @epilgrim, I believe I have solved the issue with Gettext backends not being recognised in a Cldr configuration. This commit shows the minor change required.

This release is now pushed on GitHub and published to hex as ex_cldr version 2.7.2.

Would you kindly mix deps.update ex_cldr and let me know if this issue is resolved for you?

juanperi commented 5 years ago

hey @kipcole9 just tested your new version and it works!! Thank you for your quick response, and your awesome project!