Add QueryParser#missing_value for handling missing values + tests.#2052
Merged
Add QueryParser#missing_value for handling missing values + tests.#2052
QueryParser#missing_value for handling missing values + tests.#2052Conversation
jeremyevans
requested changes
Mar 13, 2023
Contributor
jeremyevans
left a comment
There was a problem hiding this comment.
I'm fine with this in principle. One inline comment.
| def unescape(s) | ||
| Utils.unescape(s) | ||
| def unescape(string, encoding = Encoding::UTF_8) | ||
| URI.decode_www_form_component(string, encoding) |
Contributor
There was a problem hiding this comment.
This seems unrelated to the change being made. I'm not necessary opposed to this, but it shouldn't be part of this PR. It can be a separate PR with a reason given for the change. Ditto for the uri require at the top.
Member
Author
There was a problem hiding this comment.
I couldn't make the separate tests pass without it, otherwise it introduces a circular dependency. I don't want to make a separate PR as we plan to backport this AFAIK.
Contributor
There was a problem hiding this comment.
Fix for this is simple, submitted in #2055. However, considering this was backported without any discussion, I'm not sure what we want to do here.
ioquatix
added a commit
that referenced
this pull request
Mar 13, 2023
…2052) # Conflicts: # lib/rack/query_parser.rb
ioquatix
pushed 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.
ioquatix
added a commit
to socketry/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
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
dentarg
added a commit
to dentarg/sinatra
that referenced
this pull request
May 15, 2023
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
dentarg
added a commit
to dentarg/sinatra
that referenced
this pull request
May 16, 2023
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
dentarg
added a commit
to dentarg/sinatra
that referenced
this pull request
Aug 7, 2023
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
geoffharcourt
pushed a commit
to commonlit/sinatra
that referenced
this pull request
Nov 6, 2023
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
dentarg
added a commit
to dentarg/sinatra
that referenced
this pull request
Dec 23, 2023
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
dentarg
added a commit
to dentarg/sinatra
that referenced
this pull request
Jan 2, 2024
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
dentarg
added a commit
to dentarg/sinatra
that referenced
this pull request
Jan 5, 2024
Rack v3.0.7 rolled back this change with rack/rack#2059 Other references: - rails/rails#47652 - rack/rack#2052 - rack/rack#2038
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is required for customizing how
Rack::QueryParserhandles missing values and make it easy for sub-classes to change this behaviour. Based on #2038 (comment).For the purpose of Rails:
A different design option would be to make this an argument, e.g.
QueryParser.new(missing_value: nil).cc @matthewd @tenderlove