-
Notifications
You must be signed in to change notification settings - Fork 102
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
The behavior of query_keywords and query_parameters seems orthogonal, but in actuality it is not. #134
Comments
old docs around isindex tag: http://www.w3.org/MarkUp/html-spec/html-spec_7.html#SEC7.5 |
It does appear that this functionality (query_keywords) was intended to support ISINDEX. However, it indiscriminately sets query_keywords if there are no '=' signs found in the query string, when it should only be doing that if the isindex flag is set. See the decode section of https://www.w3.org/TR/html5/forms.html#url-encoded-form-data |
I did some more research into the problem. This is mainly to document my intended actions, and their reasoning. The changes that I believe are the best are as follows along with their supporting sections.
Partial isindex Support If this member was originally added to support the isindex tag, it does so in a very incomplete way. As a result, this member should not be interpretted to support the isindex tag. The special isindex encoding outlined in the HTML 5 algorithms for encoding and dedcoding application/x-www-form-urlencoded payloads [1], is applicable to both GET and POSTs according to step 18 of the Form submission algorithm [2]. No body_keywords member is present, and query_keywords will not be populated if the body is absent of the equal sign. The query_keywords member is populated through the prepare_query_parameter subroutine defined in Engine.pm. This subroutine only uses the query string. Furthermore, the encoding algorithm states the isindex value may exist with other name-value pairs. It should only be encoded/decoded as such if it is first in the form, query string, or request body. This means that the absence of the equal sign does not determine the use of the isindex tag. The History of query_keywords The query_keywords member was added with the following commit.
It was documented as behaving as follows.
All changes happening since then have been for the following reasons.
None of those changes made query_keywords more or less supportive of the isindex tag. The query_keywords member is documented as being release with 5.7008 [3].
John Napiorkowski indicated that an alternative intention for the query_keywords could not be determined through various talks in #catalyst. Before query_keywords was present, the absence of an equal sign in the query string would prevent query_parameters from being populated. Users would have to rely on the $c->req->{uri}->query instead. Fading isindex Support The isindex element was deprecated in HTML 4.01 [4]. Browsers are moving towards no longer supporting it [5]. See section Partial isindex Support. Consistency Over isindex Support The isindex tag is only treated specially if it is the first entry in the form data set. Otherwise, it is encoded as normal. This special treatment causes the value of the isindex tag to be added to the query string or payload without an equal sign, or name. When it is decoded, the string found by splitting it is meant to be treated as a value, not a name [1]. Perl provides the query_parameters and body_parameters as a hash. Hashes do not support using undefined as a key. As a result, it would require changing the return type of those members. Instead, Catalyst should choose to skip special treatment when the isindex flag is set, and decode it as a name with an undefined value.
would produce: $c->req->query_parameters = { x => undef, a = 1, b = 2 }
would produce: $c->req->query_parameters = { x => undef } References [1] - https://www.w3.org/TR/html5/forms.html#url-encoded-form-data |
HTTP::Body which appears to be responsible for parsing the request body does not appear to support isindex. A name-value pair must have a name and a value for it to be saved. |
Been using Catalyst for quite some time, and have found it to be a really great framework. Unfortunately, I came across something the other day that seemed a little confusing. I feel a code change, or documentation change may be appropriate.
I am using version 5.90092, but I believe the behavior is the same on the latest.
A data dump of the following:
my( $res, $c) = ctx_request(GET '/test_psgi_keys?x&a=1&b=2');
produces:
$c->req->query_parameters = { x => undef, a = 1, b = 2 }
$c->req->query_keywords = undef
A data dump of the following:
my( $res, $c) = ctx_request(GET '/test_psgi_keys?x');
produces:
$c->req->query_parameters = {}
$c->req->query_keywords = "x"
I feel like in the second case that the query_parameter member should instead be:
$c->req->query_parameters = { x => undef }
The query_keywords documentation states:
"Contains the keywords portion of a query string, when no '=' signs are present."
The query_parameters documentation states:
"Returns a reference to a hash containing query string (GET) parameters. Values can be either a scalar or an arrayref containing scalars."
The query_keywords member appears to have been created to support the isindex element, which is going through various states of deprecation. I understand there are a lot of finer points to this discussion, but I felt it pertinent to open this up for discussion.
The text was updated successfully, but these errors were encountered: