trailblazer / reform-rails

Automatically load and include all common Rails form features.
MIT License
99 stars 49 forks source link

Dry-Validation: Strange i18n behavior #57

Open micdahl opened 6 years ago

micdahl commented 6 years ago

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

2. locale:

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

3. Base class:

require 'reform' require 'reform/form/dry'

module MyApp class Contract < Reform::Form include Dry end end

4. 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
siassaj commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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