Support Rails 8.0 #152
Support Rails 8.0 #152
Conversation
|
Hello, |
|
I've been on leave (today is Sunday NZ local time) and will get to this next week, but there's a big backlog of other stuff. You can at least work off a branch via your Gemfile in the mean time, but it should only be a few days longer. I'll try to prioritise this specific PR. |
There was a problem hiding this comment.
Appreciate the drive-by in base.rb, thanks.
I think that Rails version check needs relaxing. We have no reason for incompatibility between Rails 7 and 8, unlike 6 to 7 which has caused the endless tedium of maintaining the v1 branch - I definitely do not want to worsen that by having to maintain a V2 branch for Rails 7, and bumping Scimitar to major version 3 for Rails 8.
I'm fairly confident that Scimitar works without modifications on Rails 7.0 or later.
| @@ -22,7 +22,7 @@ Gem::Specification.new do |s| | |||
|
|
|||
| s.required_ruby_version = '>= 2.7.0' | |||
|
|
|||
| s.add_dependency 'rails', '~> 7.0' | |||
| s.add_dependency 'rails', '~> 8.0' | |||
There was a problem hiding this comment.
This means people MUST use Rails 8. We certainly have no intention of upgrading yet given how buggy 7.1 and 7.2 have been for us - it feels like the Rails team is rushing and it's been a very small time between Rails 7 and 8. so we're being very cautious.
I think you want something like >= 7.0 in there?
| @@ -75,6 +75,7 @@ def initialize(options={}) | |||
| # attributes of this complex type object. | |||
| # | |||
| def as_json(options={}) | |||
| options = options.dup if options.frozen? | |||
| options[:except] ||= ['errors'] | |||
| super.except(options) | |||
There was a problem hiding this comment.
This is Rails 8 making five minute tasks take hours, again. Rails 7 and 8 have felt like the "bad old days" of Rails 2/3/4 era massive changes in APIs and constant dev churn. I can't find many references to frozen-by-default Hashes anywhere inside the higher level Rails 8 changes, yet it's a major change that (as we see here) can cause breakage.
In any case, I've just realised that this code was broken anyway. The "correct" #as_json takes some finding, but the relevant #as_json's options include except:. That's what is being manipulated on line 78.
So we now call super, without parens so it gets passed any and all parameters including our options. The result is a Hash. We're then calling Hash#except, but that takes the keys to exclude as a list/Array, yet we're providing an options Hash with who-knows-what as the keys. Besides, since the options were already passed in, exclusions already happened.
I'd like to contribute some minor changes to your branch to get things working for Rails 7/8 and have CI pass (for which I think I might be forced to concede auto-test for Ruby 3.2 or later only and drop tests for earlier versions, even though they still work perfectly). For example, this change - and there's one other place something similar isn needed, if tests are to pass - becomes, in my preferred variation after trying a few:
def as_json(options={})
exclusions = options[:except] || ['errors']
super(options.merge(except: exclusions))
endAre you able and willing to grant me push permission for the changes, per this reference? TIA!
There was a problem hiding this comment.
@terracatta ...ah, yay, it can be done for old Rubies by modifying the Rails constraint by version. It looks like this: #154
|
@terracatta realise you may be busy or on holiday yourself 😂 but just checking back to make sure the PR doesn't get too stale. |
|
Closing in favor of #154 |
Bumps the gemspec for Rails 8. I also added to add some consideration for frozen hashes which seem to be submitted to the
as_jsonoptions.