Skip to content
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

Make query parameters without = have nil values #2059

Merged
merged 5 commits into from Mar 16, 2023

Conversation

jeremyevans
Copy link
Contributor

This was Rack's historical behavior. While it doesn't match
URL spec section 5.1.3.3, keeping the historical behavior avoids
all of the complexity required to support the URL spec standard
by default, but also support frameworks that want to be backwards
compatible.

This reverts the four recent commits that added a lot of additional
complexity just to get the backwards compatible behavior. While
I think the URL spec behavior is better, I think it's better to have
the backwards compatible behavior without the complexity than
the URL spec behavior with all of the complexity required to also
allow frameworks to get the backwards compatible behavior.

This keeps as much of the specs added by the recently reverted
commits that make sense.

This was Rack's historical behavior.  While it doesn't match
URL spec section 5.1.3.3, keeping the historical behavior avoids
all of the complexity required to support the URL spec standard
by default, but also support frameworks that want to be backwards
compatible.

This keeps as much of the specs added by the recently reverted
commits that make sense.
Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dentarg dentarg mentioned this pull request Mar 16, 2023
7 tasks
@ioquatix
Copy link
Member

ioquatix commented Mar 16, 2023

I think what we've identified here is that the way Rack::Request works, has a lot of shortcomings, and trying to make it more standards compliant revealed that we had a ton of assumptions which don't hold true (order dependent) when sub-classes change the behaviour. The level of complexity required to make this work was insane and we've simply designed to roll it all back. Given that the previous behaviour was a superset of Rack 3, that isn't a compatibility issue (although internal implementation and some public methods are changed as part of the roll back).

I think we should consider those assumptions carefully and attempt to build something more robust going forward. Now we know a lot more about the different assumptions and ways we can address them, I think we should consider this for Rack 4.

@tenderlove
Copy link
Member

I think the URL spec behavior is better, I think it's better to have
the backwards compatible behavior without the complexity than
the URL spec behavior with all of the complexity required to also
allow frameworks to get the backwards compatible behavior.

💯💯💯

@ioquatix ioquatix merged commit 77cf057 into rack:main Mar 16, 2023
14 checks passed
ioquatix added a commit to ioquatix/rack that referenced this pull request Mar 16, 2023
* Revert "Prefer to use `query_parser` itself as the cache key. (rack#2058)"

This reverts commit 5f90c33.

* Revert "Fix handling of cached values in `Rack::Request`. (rack#2054)"

This reverts commit d25fedd.

* Revert "Add `QueryParser#missing_value` for handling missing values + tests. (rack#2052)"

This reverts commit 59d9ba9.

* Revert "Split form/query parsing into two steps (rack#2038)"

This reverts commit 9f059d1.

* Make query parameters without = have nil values

This was Rack's historical behavior.  While it doesn't match
URL spec section 5.1.3.3, keeping the historical behavior avoids
all of the complexity required to support the URL spec standard
by default, but also support frameworks that want to be backwards
compatible.

This keeps as much of the specs added by the recently reverted
commits that make sense.
# Conflicts:
#	lib/rack/multipart.rb
#	lib/rack/request.rb
#	test/spec_request.rb
ioquatix added a commit that referenced this pull request Mar 16, 2023
* Revert "Prefer to use `query_parser` itself as the cache key. (#2058)"

This reverts commit 5f90c33.

* Revert "Fix handling of cached values in `Rack::Request`. (#2054)"

This reverts commit d25fedd.

* Revert "Add `QueryParser#missing_value` for handling missing values + tests. (#2052)"

This reverts commit 59d9ba9.

* Revert "Split form/query parsing into two steps (#2038)"

This reverts commit 9f059d1.

* Make query parameters without = have nil values

This was Rack's historical behavior.  While it doesn't match
URL spec section 5.1.3.3, keeping the historical behavior avoids
all of the complexity required to support the URL spec standard
by default, but also support frameworks that want to be backwards
compatible.

This keeps as much of the specs added by the recently reverted
commits that make sense.
# Conflicts:
#	lib/rack/multipart.rb
#	lib/rack/request.rb
#	test/spec_request.rb
dentarg added a commit to dentarg/sinatra that referenced this pull request May 15, 2023
dentarg added a commit to dentarg/sinatra that referenced this pull request May 15, 2023
dentarg added a commit to dentarg/sinatra that referenced this pull request May 16, 2023
dentarg added a commit to dentarg/sinatra that referenced this pull request Aug 7, 2023
geoffharcourt pushed a commit to commonlit/sinatra that referenced this pull request Nov 6, 2023
dentarg added a commit to dentarg/sinatra that referenced this pull request Dec 23, 2023
dentarg added a commit to dentarg/sinatra that referenced this pull request Jan 2, 2024
dentarg added a commit to dentarg/sinatra that referenced this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants