-
Notifications
You must be signed in to change notification settings - Fork 1.4k
in_http: replace WEBrick::HTTPUtils.parse_query with URI #4900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WEBrick is no longer recommended for production use. WEBrick::HTTPUtils is just a util, so it might be harmless. However, it would be happy to reduce dependence if possible. Signed-off-by: Daijiro Fukuda <[email protected]>
@@ -643,6 +643,10 @@ def include_cors_allow_origin | |||
|
|||
!r.nil? | |||
end | |||
|
|||
def parse_query(query) | |||
query.nil? ? {} : Hash[URI.decode_www_form(query, Encoding::ASCII_8BIT)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
';' separator to be ignored. Is it ok here?
(I'm not sure difference WEBrick::HTTPUtils.parse_query and URI.decode_www_form)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't notice that.
If we want to keep that specification, it would be better not to replace it by force.
WEBrick::HTTPUtils
is just a util, so it might be harmless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a breaking change, but it might be acceptable for v1.19.
Should we move forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should give it some time.
We want to remove runtime dependence on WEBrick, but I don't see using WEBrick::HTTPUtils
as a problem because it is just utils.
If more people are not happy with this change of specifications, then it might be better not to include this modification.
Which issue(s) this PR fixes:
What this PR does / why we need it:
Replace
WEBrick::HTTPUtils.parse_query
withURI.decode_www_form
inin_http
.WEBrick is no longer recommended for production use.
WEBrick::HTTPUtils
is just a util, so it might be harmless.However, it would be happy to reduce dependence if possible.
This changes some specifications unintentionally.
At least, this makes us unable to use
;
delimitter.We need to consider whether this breaking change is acceptable or not.
Docs Changes:
Not needed. (There is no impact on the specification.)
Release Note:
The same as the title.