Update HTTP status codes and associated symbols#2137
Conversation
|
This looks reasonable to me. @jeremyevans are you happy that backwards compatibility is sufficient? |
jeremyevans
left a comment
There was a problem hiding this comment.
I'm good with the general idea.
|
I removed the duplicated constant and added deprecation warnings. The use of |
ioquatix
left a comment
There was a problem hiding this comment.
Looking good, just a few minor suggestions.
|
Thank you for feedback. I've made the following changes:
|
jeremyevans
left a comment
There was a problem hiding this comment.
This looks almost ready. I just have a couple of minor change requests.
…obsolete or non-standard reason phrases
|
Can we introduce the new names but keep the old names not deprecated? For a libraries on top of rack the only way to support Rack 2 and Rails 3 at the same time after this change is by introducing a bunch of conditionals on the version of rack. |
|
@rafaelfranca feel free to create a PR, not completely sure what issue you are having but sounds reasonable. |
|
Will do. The problem is that in Rails we have a few controllers with
or some variation of that. I'm also considering dropping support to Rack 2, in that case the problem will be solved for Rails 8, but not for 7.1, but I can probably work around the issue itself in Rails since that code will not live for much loger. |
|
I believe you can use the |
|
That shows the deprecation. We don't want Rails to show people deprecation message for code the framework has. |
|
In my view, it's best to keep the deprecation message in Rack. Frameworks like Rails can manage the transition by adding status code symbols: if Gem::Version.new(Rack.release) >= Gem::Version.new('3.y.z')
Rack::Utils::SYMBOL_TO_STATUS_CODE[:payload_too_large] = 413
Rack::Utils::SYMBOL_TO_STATUS_CODE[:unprocessable_entity] = 422
end
|
|
This is exactly my point. I don't think rack should be pushing frameworks to do version checks. Rack is supposed to be a stable API. If a framework that depends on rack needs to check a version of the library we can't consider Rack stable anymore. I'm all for adding features to Rack, but all their changes need to be backward compatible in a way that users don't have a hard time supporting 2 versions of Rack. So my suggestion is: while Rack 2 is still supported, we don't show any deprecation, when only Rack 3 is supported we can add the deprecation in some Rack 3.x and in Rack 4 we remove it. If this plan was hard to implement, I'd totally understand pushing version checks to users, but I'm sure we can keep an old status code name longer to make users' life easier. |
|
Just to be clear, not criticizing the breaking changes/deprecations we had to do to evolve the spec, fix issues, etc, just saying there are a few like this one we can avoid if we decide to. |
I don't think we should consider mutating another library's constants to be "part of the public API". It's true that the constant is not frozen, but I also consider it bad manners to mutate someone else's constants. Also, in general, I think it's acceptable to freeze a constant but not do a major version release. IOW constant mutation is not public API, please do not consider it as such. If we want an API to mutate this constant, then we should add methods people call to register keys and values.
I think this is reasonable. |
…processable_content" This fixes rack deprecation warning rack/rack#2137
|
This seems to cause issues to dependencies like Rails (fixed in #2209) but also RSpec, see rspec/rspec-rails#2763. |
Ref: rack/rack#2137 Rackをupdateしたため、その対応が必要になった
rack/rack#2137 Corrige les tests qui échouent dans #1365, #1366, et #1367
rack/rack#2137 Corrige les tests qui échouent dans #1365, #1366, et #1367
|
I don't quite undertand why I started getting deprecation messages only on upgrading to Rack 3.2 -- did not get them in Rack 3.1, did get them in Rack 3.2. Is that the intent? Reading the comments here, it does not seem to be the intent, but it's what's going on for me? On Rails 8.0.2.1 code like Deprecation notice in Rack 3.2, not in Rack 3.1. |
|
IIRC, we decided to not emit deprecation warnings in 3.1 because it was too noisy, to give Rails a chance to do something about it. |
These commits bring Rack in sync with the 2022-06-08 version of the IANA HTTP Status Code Registry.
This change is notably impactful in that the symbol for HTTP status code 422 changes from
:unprocessable_entityto:unprocessable_content. To mitigate the impact, I added legacy symbol lookup toRack::Utils.status_code.It seems that some Rubyists are in the habit of retrieving status codes from
Rack::Utils::SYMBOL_TO_STATUS_CODEdirectly (examples: here, here, here) rather than using the public method. Admittedly, I've done this myself. Well, it can't be helped, but perhaps this pattern can be discouraged in the future.