Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dry-Validation: Strange i18n behavior #57

Open
micdahl opened this issue Sep 29, 2017 · 10 comments
Open

Dry-Validation: Strange i18n behavior #57

micdahl opened this issue Sep 29, 2017 · 10 comments

Comments

@micdahl
Copy link

micdahl commented Sep 29, 2017

Complete Description of Issue

I am using my own base class MyApp::Contract < Reform::Form for all my Reform-Objects and have configured Dry::Validation::Schema to use i18n messages. My locale is de. If I submit my form with invalid data, I don't get the correct local error message but the default en one. If I just edit and save my base class with just adding or deleting a blank line while puma is running and resubmit the form, I get the correct de locale. I am not sure if this bug is in Reform, Rails, Puma or wherever.

Update: Editing and saving the derived class has the same result
Update: Feature Testing with cucumber leads to wrong localization, too. So Puma seems not to be the problem.
Update: Even when changing the derived class as follows, I get the same behavior:

module User::Contract
  class Create < MyApp::Contract
    property :forename
  
    validation do
      configure { config.messages = :i18n }
      required(:forename).filled
    end
  end
end

Update: When overwriting

en:
  errors:
    filled?: "bitte ausfüllen"

I get this german text. So there seems to be use of i18n, but the wrong locale is used.

Steps to reproduce

  1. initializer for dry-validation and locale:
module MyApp
  class Application < Rails::Application
    config.i18n.load_path += Dir[Rails.root.join('config', 'locales', '**', '*.{rb,yml}')]
    config.i18n.default_locale = :de
  end
end

Dry::Validation::Schema.configure do |config|
  config.messages = :i18n
end

Dry::Validation::Schema::Form.configure do |config|
  config.messages = :i18n
end
  1. locale:
de:
  errors:
    filled?: "bitte ausfüllen"
  1. Base class:
require 'reform'
require 'reform/form/dry'

module MyApp
  class Contract < Reform::Form
    include Dry
  end
end
  1. Concrete class:
module User::Contract
  class Create < MyApp::Contract
    property :forename

     validation do
      required(:forename).filled
    end
  end
end

Expected behavior

Error message should be bitte ausfüllen when form is submitted without filling forename

Actual behavior

Error message is must be filled. After I edit and save the file where the base class is in (step 3. from above) while puma is running, bitte ausfüllen is shown after submit as expected.

System configuration

Reform version: 2.2.4
Reform Rails version: 0.1.7
Trailblazer Rails version: 0.1.7
Rails version: 5.1.4

@micdahl micdahl changed the title Dry-Validation: Strange i18n behaveiour Dry-Validation: Strange i18n behavior Sep 29, 2017
@siassaj
Copy link

siassaj commented Sep 29, 2017

Ok, i'm trying to debug this.

I18n.locale = :de
dry_validation_schema = User::Contract::Create.validation_groups.first.second.instance_variable_get(:@validator)
dry_validation_schema.call(forename: '"')
=> #<Dry::Validation::Result output={:forename=>""} errors={:forename=>["bitte ausfüllen"]}>

So that schema's good

form = User::Contract::Create.new(OpenStruct.new)
form.validate(forename: "")
form.errors.messages
=> {:forename=>["must be filled"]}

So reform's doing some weirdness.
I'll investigate more :)

@fran-worley
Copy link
Contributor

@siassaj form memory, if you want to generate messages in a schema other than the default, you pass the locale into the dry-v messages call.

Currently in Reform there is no handling for passing options to the dry-v messages call and we do that internally.

@siassaj
Copy link

siassaj commented Sep 29, 2017

@fran-worley I edited the message. From dry-v's own docs, if dry-v is loaded after i18n gem then it doesn't need for the locale to be passed in.

Though, is there a reason that reform can't do

schema.call(<stuff>).messages(locale: I18n.locale)

@siassaj
Copy link

siassaj commented Sep 29, 2017

Ok, I've found the actual problem;

Examine closely the following weirdness:

module User::Contract
  class Create < MyApp::Contract
    property :forename
  
    validation do
      configure { config.messages = :i18n }
      required(:forename).filled
    end
  end
end

I18n.locale = :de

form = User::Contract::Create.new(OpenStruct.new)
form.validate(forename: "")
form.errors.messages
=> {:forename=>["must be filled"]}

vs

I18n.locale = :de

module User::Contract
  class Create < MyApp::Contract
    property :forename
  
    validation do
      configure { config.messages = :i18n }
      required(:forename).filled
    end
  end
end

form = User::Contract::Create.new(OpenStruct.new)
form.validate(forename: "")
form.errors.messages
=> {:forename=>["bitte ausfüllen""]}

This is because of dry-validations;

module User::Contract
  class Create < MyApp::Contract
    property :forename
  
    validation do
      configure { config.messages = :i18n }
      required(:forename).filled
    end
  end
end

I18n.locale = :de

dry_validation_schema = User::Contract::Create.validation_groups.first.second.instance_variable_get(:@validator)
dry_validation_schema.call(forename: "")
=> #<Dry::Validation::Result output={:forename=>""} errors={:forename=>["bitte ausfüllen"]}>
dry_validation_schema.call(forename: "").messages
=> {:forename=>["must be filled"]}

vs

I18n.locale = :de

module User::Contract
  class Create < MyApp::Contract
    property :forename
  
    validation do
      configure { config.messages = :i18n }
      required(:forename).filled
    end
  end
end

dry_validation_schema = User::Contract::Create.validation_groups.first.second.instance_variable_get(:@validator)
dry_validation_schema.call(forename: "")
=> #<Dry::Validation::Result output={:forename=>""} errors={:forename=>["bitte ausfüllen"]}>
dry_validation_schema.call(forename: "").messages
=> {:forename=>[""bitte ausfüllen"]}

In a silly twist, dry-validations does this nonesense:

dry_validation_schema.call(forename: "")
=> #<Dry::Validation::Result output={:forename=>""} errors={:forename=>["bitte ausfüllen"]}>
dry_validation_schema.call(forename: "").messages
=> {:forename=>["must be filled"]}
dry_validation_schema.call(forename: "").errors
=> {:forename=>["bitte ausfüllen"]

So, dry-validations is holding onto localization data from the moment we register the validation group. More clearly, when the reform class is put into memory.

I have dry-validations 0.10.7, and this line show's what's going on.
https://github.com/dry-rb/dry-validation/blob/v0.10.7/lib/dry/validation/message_compiler.rb#L18
It's the same in the most recent version of dry-validations so it should exhibit the same behaviour, though that should be clarified.

So dry-validation's support of I18n gem, specifically setting the global but thread local '#locale'.

Similar problem here, possibly the same issue
https://github.com/dry-rb/dry-validation/issues/245

@micdahl
Copy link
Author

micdahl commented Sep 29, 2017

Thanks for your fast investigations!
So as a user, you can do

require "i18n"
I18n.locale = :de
Dry::Validation::Schema.configure do |config|
  config.messages = :i18n
end

Dry::Validation::Schema::Form.configure do |config|
  config.messages = :i18n
end

in a config/initialization/ file to get a defined locale running at the moment at least.

@fran-worley
Copy link
Contributor

@micdahl unless you're using custom base schemas, you probably don't need the:

Dry::Validation::Schema::Form.configure do |config|
  config.messages = :i18n
end

As Reform doesn't use it see

@siassaj
Copy link

siassaj commented Sep 29, 2017

@micdahl yeah but it's a silly situation. Changing I18n.locale really ought to change the error messages you get without having to dance through hoops.

I've left a message in dry-v issue tracker to see what we can do about this. It's so weird.

@siassaj
Copy link

siassaj commented Sep 29, 2017

@fran-worley

MyForm.validation_groups.first.second.instance_variable_get(:@validator).class.superclass
=> Dry::Validation::Schema::Form

Seems to be using it.

In any case it works, I had to do this in my initializers

# dry-validation ought to use i18n for it's error messages
# so set that up here per docs
Dry::Validation::Schema.configure do |config|
  config.messages = :i18n
  config.predicates = MyPredicates
end

# Reform uses Schema::Form for it's validations, and for some reason
# setting the config above doesn't actually translate into Form.
# It's likely that any class namespaced under Schema will exhibit
# this problem, as configuration seems to be set on Schema,
# not in anything like Dry::Validation::Configuration.
Dry::Validation::Schema::Form.configure do |config|
  config.messages = :i18n
  include MyPredicates
end

@fran-worley
Copy link
Contributor

@siassaj the latest version of reform (2.3.0rc1) doesn't. We purposefully changed it to use Dry::Validation::Schema as Reform handles coercion itself (via disposable/ dry-types) and using Form with already coerced values was causing all sorts of unexpected mess!

@smaximov
Copy link

@siassaj, your issue may be related to mine, see dry-rb/dry-validation#368 for possible workarounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants