Make query parameters without = have empty string values #2098
Conversation
|
Is this going to ship with Rack 3.1? |
|
I suggest we consider this for Rack 3.2+ or 4.0. Maybe we can introduce |
|
I would like this behavior in 3.1. This was the original Rack 3.0.0 behavior. We compromised our integrity to change the behavior to be the same as 2.2 in response to complaints from Rails. Then we merged the form_pairs support, also in response to complaints from Rails, to give Rails the control it wants. Now that the form_pairs commit has been merged, we can change back to the behavior desired and shipped in 3.0.0, and Rails can use form_pairs to implement the behavior it wants. |
|
I think this is good, but I'd like input from @matthewd |
|
@jeremyevans sorry to impose, but do you mind giving a brief summary of the advantages of this change again? |
| @@ -129,6 +129,8 @@ def normalize_params(params, name, v, _depth=nil) | |||
|
|
|||
| return if k.empty? | |||
|
|
|||
| v ||= String.new | |||
There was a problem hiding this comment.
At one point, I think we made this configurable using a method - is that still worth considering? I liked that approach.
There was a problem hiding this comment.
I don't think so. There isn't a strong need for it now that form_pairs can be used for custom parameter parsing.
Compliance with the URL spec, section 5.1.3.3. You agreed that this behavior is correct: #1696 (comment) . This is how the browser Javascript methods handle things: #1696 (comment) |
|
While I agree it's correct according to the spec, I'm worried about things like #1696. |
|
I am okay with this PR, but would like @matthewd to confirm that this would be reasonable for Rack 3.1. I'd also like @tenderlove to approve it too if possible. This one has a bigger chance for backwards compatibility issues, so I'd like everyone to be clear on that. |
This was reverted in 77cf057 so that Rails could have historical Rack behavior without the complexity of splitting form/query parsing. Now that splitting form/query parsing has been added back in 3855d1d, this changes the behavior to what originally shipped in Rack 3. This matches URL spec section 5.1.3.3. Frameworks that want Rack's historical behavior of using nil values instead of empty string values can reparse QUERY_STRING and use rack.request.form_pairs to get the behavior they want.
ioquatix
force-pushed
the
query_parameter_without_value
branch
from
d28fa2a
to
6f09faf
Compare
|
So, my obvious concern is going to be that while Rails now has access to APIs to do its own parameter parsing, no existing releases are actually doing that. I can fix that for the incipient 7.2, but I do worry that people might upgrade existing 7.1 applications to the latest Rack without much concern for edge cases outside their test coverage. ... and I now realise that 7.1 has a completely open-ended dependency on Rack, so that's somewhat of a permanent problem [arguably of our own making]. Given an overabundance of druthers, I might hope we could have a medium-sized [or at least non-zero] window where "latest-available Rails does its own back-compat param parsing" precedes "latest-available Rack diverges from previous behaviour"... but I don't think that's really a reasonable thing for me to ask. I agree that making it configurable is unnecessary given we have a more holistic solution available... one option would be to just extract it to a method, though -- to at least allow any affected applications (Rails or otherwise) to monkey-patch their way back to historical compatibility without having to reproduce all of (I still think it's weird for us to make this change given that all our array/hash parsing is, by definition & design, wildly non-conformant -- but I accept that we're past the question of whether it should happen.) |
|
If that's the case we should probably punt this to a future release of Rack. |
OK with me. |
|
@matthewd can you please advise us when Rails has got a fix in place, and then at the next major release of Rails, we have the option to adopt this PR. IOW, we will wait at least one major Rails release with the fix, before releasing a major release of Rack with this change. |
This was reverted in 77cf057 so that Rails could have historical Rack behavior without the complexity of splitting form/query parsing.
Now that splitting form/query parsing has been added back in 3855d1d, this changes the behavior to what originally shipped in Rack 3.
This matches URL spec section 5.1.3.3. Frameworks that want Rack's historical behavior of using nil values instead of empty string values can reparse QUERY_STRING and use rack.request.form_pairs to get the behavior they want.