Support Ruby 3+ and Rails 7+ #612
Conversation
Improved cache attachments parsing
Extracted from here refile#612
|
Can this be merged? We'd like to migrate everything to ActiveStorage, but Refile is still keeping us on Ruby 2.7 in the meantime. |
| if send(attacher).present? | ||
| send(attacher).valid? | ||
| errors = send(attacher).errors | ||
| errors.each do |error| | ||
| self.errors.add(name, *error) | ||
| errors.each do |error_type, error| | ||
| self.errors.add(name, error_type, **(error || {})) | ||
| end | ||
| end |
There was a problem hiding this comment.
[Question/Suggestion] Since I'm interested in your fix for this issue (which is happening on the application I work on as well -- Rails 6), I need to ask you this: are you sure this is going to work fine on Rails 7? I was checking its source code and the ActiveModel::Errors#each method implementation is actually a method delegation to its @errors instance variable, which is a simple Array instance. So it looks like your changes wouldn't work well on Rails 7.
Nonetheless, it seems your implementation is actually based on Rails 5, where ActiveModel::Errors#each method implementation does expect a two parameters arity block, as you can see on the link I just shared. On the other hand, Rails 6's implementation of the same method will display a deprecation warning message when a block with two parameters arity is provided, as you can see here in its source code.
So, having all that said, it's also worth to mention that we could go beyond the former version of this code and use ActiveModel::Errors#import method that is present on Rails 6+ (and Rails 7+) and use a fallback to the add method for older Rails versions.
Therefore, all things considereds, the change suggestion below should work at least on Rails 4+ to 7+:
| if send(attacher).present? | |
| send(attacher).valid? | |
| errors = send(attacher).errors | |
| errors.each do |error| | |
| self.errors.add(name, *error) | |
| errors.each do |error_type, error| | |
| self.errors.add(name, error_type, **(error || {})) | |
| end | |
| end | |
| next if (attacher_record = send(attacher)).nil? || attacher_record.valid? | |
| # {ActiveModel::Errors#each} will yield only one parameter, a {ActiveModel::Error} object | |
| # as `attr_or_error` on Rails 6+ (and 7+), and two parameters on Rails < 6, with | |
| # `attr_or_error` being a {Symbol} object and `msg` being a {String} object. | |
| attacher_record.errors.each do |attr_or_error, msg| | |
| msg && errors.add(name, attr_or_error, message: msg) || | |
| errors.import(attr_or_error, attribute: name) | |
| end |
|
|
||
| .idea |
|
@leandro Fwiw, we decided to speed up the process of migrating to ActiveStorage. Using parallel jobs it was pretty doable with hundreds of thousands of uploads. |
Gotcha! Thanks for the heads up, Bram. I might then open a new PR addressing that specific bug and an Issue associated to the new PR. |
|
Hey folks, have you some ideia that when you send this to production? |
|
Will resolve the problem that keyword: object? #618 |
No description provided.