Skip to content
New issue

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

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

Already on GitHub? # to your account

Reform::Rails doesn't work with Rails 6.1 well #86

Open
y-yagi opened this issue Dec 17, 2020 · 7 comments · Fixed by #88
Open

Reform::Rails doesn't work with Rails 6.1 well #86

y-yagi opened this issue Dec 17, 2020 · 7 comments · Fixed by #88

Comments

@y-yagi
Copy link
Contributor

y-yagi commented Dec 17, 2020

When using Reform::Rails with Rails 6.1, error messages of form can't acquire.

This is because Reform::Contract::Result expected that error messages are Hash or Array.
https://github.com/trailblazer/reform/blob/bcae40b7405a9ac3ee46507fa2c85acbc997a8c0/lib/reform/result.rb#L37

But in Rails 6.1, error messages are instances of ActiveModel::DeprecationHandlingMessageArray(this due to show deprecate message. Ref: rails/rails#32313). So error messages are all ignored.

This can fix with convert value to Array before check is_a?(Array) . But I think that may not a good approach...

@marcelolx
Copy link
Contributor

Maybe a better approach will be reform-rails have its own implementation of filter_for to handle with the new ActiveModel errors API and keep using reform implementation for rails < 6.1 🤔

@cherbst-2112
Copy link

Tried this idea, seems to work as a monkey patch:

module Reform
  class Contract < Disposable::Twin
    class Result

      private

      # this doesn't do nested errors (e.g. )
      def filter_for(method, *args)
        @results.collect { |r| r.public_send(method, *args).to_h }
                .inject({}) { |hah, err| hah.merge(err) { |key, old_v, new_v| (new_v.is_a?(Array) ? (old_v |= new_v) : old_v.merge(new_v)) } }
                .find_all { |k, v| # filter :nested=>{:something=>["too nested!"]} #DISCUSS: do we want that here?
                  if v.is_a?(Hash)
                    nested_errors = v.select { |attr_key, val| attr_key.is_a?(Integer) && val.is_a?(Array) && val.any? }
                    v = nested_errors.to_a if nested_errors.any?
                  end
                  v.is_a?(ActiveModel::DeprecationHandlingMessageArray)
                }.to_h
      end
    end
  end
end

Testing for a class that is only temporarily supposed to be there to serve deprecation warnings seems like a hack, but it does work as a starting point

@schorsch
Copy link

schorsch commented Feb 4, 2021

@cherbst-2112 thanks for the patch, i added it as an initializer and can confirm that it works well .. at least my tests say so :-)

@apotonick
Copy link
Member

What a super annoying bug! 💣 Just ran into it myself.

@tpendragon
Copy link

Had to apply the patch too, any status updates here? Should I assume this is broken and AM support is going away?

@apotonick
Copy link
Member

@tpendragon AM is not going to be dropped in Reform, but we're working on Reform 3 and this fell through the cracks. Will review today with @yogeshjain999 💪

@tpendragon
Copy link

Y'all are the best, thank you!

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

Successfully merging a pull request may close this issue.

6 participants