Re: [RFC] Add parse_query_string as an alternative to parse_str

  115546
July 22, 2021 02:10 tobias.nyholm@gmail.com (Tobias Nyholm)
--Apple-Mail=_909882CD-D624-4E37-BC1B-41D20C182625
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

Hey.=20

I see the RFC [https://wiki.php.net/rfc/parse_str_alternative =
<https://wiki.php.net/rfc/parse_str_alternative>] is just a rename of =
parse_str()

I agree with this. parse_str() is a really confusing name and it does =
not behave as I expect it to. I very much support changing it to =
parse_query_string() and make sure it gets a return value.=20

// Tobias


PS. I hope this message will add as a thread on =
https://news-web.php.net/php.internals/115081 =
<https://news-web.php.net/php.internals/115081>.=20=

--Apple-Mail=_909882CD-D624-4E37-BC1B-41D20C182625--
  115633
August 5, 2021 22:21 tekiela246@gmail.com (Kamil Tekiela)
Hi Internals,

I have added implementation for
https://wiki.php.net/rfc/parse_str_alternative. If there are no other
comments, I would like to start voting on Saturday.

Regards,
Kamil
  115635
August 6, 2021 07:45 mike@newclarity.net (Mike Schinkel)
> On Aug 5, 2021, at 6:21 PM, Kamil Tekiela <tekiela246@gmail.com> wrote: > > I have added implementation for > https://wiki.php.net/rfc/parse_str_alternative. If there are no other > comments, I would like to start voting on Saturday.
I too would appreciate having a function in the PHP library that returns an array and that is named more intuitively than parse_str(). However, I would suggest naming it `parse_query()` instead of `parse_query_string()` as `_string()` is redundant and I see shorter function names being preferable when the intent of the function is clear. I searched for prior art and it appears that Guzzle's PSR7 helper library v1.x had a namespaced parse_query() function: https://github.com/guzzle/psr7/blob/1.x/src/functions.php#L299 <https://github.com/guzzle/psr7/blob/1.x/src/functions.php#L299> I found Psr7\parse_query() being called on GitHub in 840 places using SourceGraph code search: https://sourcegraph.com/search?q=context:global+lang:php+Psr7%5Cparse_query%28+count:all&patternType=literal <https://sourcegraph.com/search?q=context:global+lang:php+Psr7%5Cparse_query%28+count:all&patternType=literal> There was only one (1) place with SourceGraph code search where someone named a function parse_query_string(), and that example did not use parse_query_string() in the same way as your RFC: https://sourcegraph.com/github.com/k0a1a/hotglue2/-/blob/controller.inc.php?L344:1 <https://sourcegraph.com/github.com/k0a1a/hotglue2/-/blob/controller.inc.php?L344:1> My takeaway is that PHP developers will easily be able to understand the intent if named `parse_query()`, especially those who might be familiar with it from Guzzle, and thus there is no need for the redundant `_string()`. And, of course, a parse_query() in the global namespace won't conflict with Guzzle's use because their function is namespaced with Guzzle\Psr7.
> On Aug 6, 2021, at 3:17 AM, ignace nyamagana butera <nyamsprod@gmail.com> wrote: > > I feel that we are missing a chance to also improve how parse_str > algorithm is currently used, we could or should (?) use this opportunity > to fix some long shortcomings around parse_str.
Also, +1 to this. -Mike P.S. WordPress has a parse_query() *method* on their WP_Query() class, but as someone who has worked with WordPress for 10+ years even I don't see these two in conflict. One is a method scoped to the solution domain of its class and the other would be a function in PHP's global namespace.
  115636
August 6, 2021 07:51 alec@alec.pl (Aleksander Machniak)
On 06.08.2021 09:45, Mike Schinkel wrote:
> I too would appreciate having a function in the PHP library that returns an array and that is named more intuitively than parse_str(). > > However, I would suggest naming it `parse_query()` instead of `parse_query_string()` as `_string()` is redundant and I see shorter function names being preferable when the intent of the function is clear.
I agree about the _string suffix removal. However, I know we have parse_url() already, but parse_query() might be too generic. I would suggest adding "http" to the name. And as we already have http_build_query() I would rather see http_parse_query(). The RFC should target 8.2. -- Aleksander Machniak Kolab Groupware Developer [https://kolab.org] Roundcube Webmail Developer [https://roundcube.net] ---------------------------------------------------- PGP: 19359DC1 # Blog: https://kolabian.wordpress.com
  115637
August 6, 2021 08:05 mike@newclarity.net (Mike Schinkel)
> On Aug 6, 2021, at 3:51 AM, Aleksander Machniak <alec@alec.pl> wrote: > > I agree about the _string suffix removal. However, I know we have > parse_url() already, but parse_query() might be too generic. I would > suggest adding "http" to the name. And as we already have > http_build_query() I would rather see http_parse_query().
+1. http_parse_query() is even better. -Mike
  115643
August 6, 2021 14:09 scott@paragonie.com (Scott Arciszewski)
http_parse_query() would get my vote (if I had vote karma :P)

On Fri, Aug 6, 2021 at 4:05 AM Mike Schinkel <mike@newclarity.net> wrote:

> > On Aug 6, 2021, at 3:51 AM, Aleksander Machniak <alec@alec.pl> wrote: > > > > I agree about the _string suffix removal. However, I know we have > > parse_url() already, but parse_query() might be too generic. I would > > suggest adding "http" to the name. And as we already have > > http_build_query() I would rather see http_parse_query(). > > +1. http_parse_query() is even better. > > -Mike > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > >
  115645
August 6, 2021 15:29 cmbecker69@gmx.de ("Christoph M. Becker")
On 06.08.2021 at 16:09, Scott Arciszewski wrote:

> http_parse_query() would get my vote (if I had vote karma :P)
You have a php.net account[1], so you are supposed to have voting karma. :) [1] <http://people.php.net/sarciszewski> -- Christoph M. Becker
  115639
August 6, 2021 09:28 ayesh@php.watch (Ayesh Karunaratne)
> I agree about the _string suffix removal. However, I know we have > parse_url() already, but parse_query() might be too generic. I would > suggest adding "http" to the name. And as we already have > http_build_query() I would rather see http_parse_query(). >
+1 for http_parse_query() as it sounds a lot more natural and consistent.
> parse_str assumes that the query separator is always a "&" which reduces > its usage to only parsing query using that particular character. Again this > might be seen as an edge case but no RFC prevents using any other > character. If you where to use another character you are bound to use some > userland code workaround to then feed the "normalized" string to parse_str
Both `http_build_query` and `parse_str` respect the `arg_separator.input` [INI setting](https://www.php.net/manual/en/ini.core.php#ini.arg-separator.input). If we were to go for a `http_parse_query`, I think it makes sense to support the same behavior and parameters. Thank you, Ayesh.
  115644
August 6, 2021 15:06 larry@garfieldtech.com ("Larry Garfield")
On Thu, Aug 5, 2021, at 5:21 PM, Kamil Tekiela wrote:
> Hi Internals, > > I have added implementation for > https://wiki.php.net/rfc/parse_str_alternative. If there are no other > comments, I would like to start voting on Saturday. > > Regards, > Kamil
I will be voting No on this as is. Not because I'm against improving the query parsing tools; I'm in favor of that. But as noted, this RFC doesn't do enough to improve it. Returning a value and changing the name isn't enough to justify another global method. As others noted, the parsing rules themselves are fugly and need to be improved. More importantly to me, though, it is the year of our lord 2021, and PHP APIs have no business pretending that arrays with possibly-missing values are a data structure. They're a satire of a data structure. If you're parsing a string with known structure into something, that something should be a properly defined object. The new PhpToken is a good example of that shift done well. Don't give me an array where I have to mess around with isset() and crap like that. Give me a properly defined HttpQuery object with named, type-enforced properties and meaningful methods. Give it a parse(string) static method and a __toString() method to convert back to a query string. Anything less is, frankly, not worth the effort. --Larry Garfield
  115647
August 6, 2021 15:59 rowan.collins@gmail.com (Rowan Tommins)
On 06/08/2021 16:06, Larry Garfield wrote:
> Give me a properly defined HttpQuery object with named, type-enforced properties and meaningful methods. Give it a parse(string) static method and a __toString() method to convert back to a query string. Anything less is, frankly, not worth the effort.
While I understand your general principle, I'm not sure what such an object would look like. There is no "standard list" of query string keys which could be named as properties on a built-in HttpQuery object; every request is different, so the user would have to provide the object to be "hydrated" in some form. Maybe it would have to be a trait, so you could write something like this? class GetUserQueryParams {     use HttpQueryParser;     private ?int $id;     private ?string $name; } $params = GetUserQueryParam::fromQueryString('?id=42'); Or a utility method that took a class name, and converted the query string parameters to named constructor arguments? class GetUserQueryParams {     public function __construct(        private ?int $id,        private ?string $name    ) {} } $params = parse_query_string('?id=42', GetUserQueryParams::class); Once you've started down that route, people will want a way to alias property names (e.g. with attributes), have parallel handling for other formats like JSON ... and before you know it you have the kind of complexity which is much easier to manage as a userland library than something tied to core. On the other hand, if core provides a sane implementation of the parsing into a generic "bag of key-value pairs" (which PHP calls an "array"), that would provide a very useful building block for any userland library to build on. Regards, -- Rowan Tommins [IMSoP]
  115650
August 6, 2021 19:42 tekiela246@gmail.com (Kamil Tekiela)
Hi Internals,

Thanks for all the feedback. I have changed the name to http_parse_query as
it looks like more people prefer that name. I have also updated
https://wiki.php.net/rfc/parse_str_alternative for 8.2 (sorry for the
confusion) and I added open points.
I hear two concerns currently about this RFC. Please note that my intention
was to provide a simple drop-in replacement. However, I agree that there is
a lot of room for improvement in this area, so I highly appreciate your
comments on how we can best tackle these open points.
I will not open this RFC for voting any time soon, as I want to let the
conversation run through to see what other options/concerns people have.
Please feel free to share your comments on what you think is the right path
to improve this functionality in PHP.

Regards,
Kamil
  115761
August 18, 2021 12:01 guilliam.xavier@gmail.com (Guilliam Xavier)
On Fri, Aug 6, 2021 at 9:43 PM Kamil Tekiela <tekiela246@gmail.com> wrote:

> Hi Internals, > > Thanks for all the feedback. I have changed the name to http_parse_query as > it looks like more people prefer that name. I have also updated > https://wiki.php.net/rfc/parse_str_alternative for 8.2 (sorry for the > confusion) and I added open points. > I hear two concerns currently about this RFC. Please note that my intention > was to provide a simple drop-in replacement. However, I agree that there is > a lot of room for improvement in this area, so I highly appreciate your > comments on how we can best tackle these open points. > I will not open this RFC for voting any time soon, as I want to let the > conversation run through to see what other options/concerns people have. > Please feel free to share your comments on what you think is the right path > to improve this functionality in PHP. > > Regards, > Kamil >
Hi, thanks for the RFC. I agree with Rowan in https://externals.io/message/115642 that http_parse_query() should "mirror http_build_query() as closely as possible", notably: - have a similar signature (except for the first parameter `array|object $data` becoming `string $query` and the return type `string` becoming `array`, of course); - do not do name mangling (and for your question "what should happen to mismatched square brackets?": not sure without an example, but I would say "just keep them as is"). Best regards, -- Guilliam Xavier
  115763
August 18, 2021 13:27 tekiela246@gmail.com (Kamil Tekiela)
Hi Internals,

During my research into this topic, I discovered that there exists a
multibyte variant of this function in mbstring extension. This raises the
question: should we add a corresponding multibyte variant of
http_parse_query() to mbstring? Is there any usage in the wild of
mb_parse_str()?
I have even asked a question on Stack Overflow about this topic
https://stackoverflow.com/questions/68761695/can-mb-parse-str-produce-different-results-than-parse-str

To me, it doesn't look like parse_str() is useful anymore, but I come from
Europe where things happen mostly using a single encoding. I am not aware
of any problems that parse_str() might have when it comes to multiple
encoding types.
In fact, when looking at the whole input/output encoding feature of
mbstring I am getting a feeling that this is some niche feature that almost
nobody ever uses. While mbstring is useful for dealing with different
encodings, I am not sure if it is common for HTTP requests to be encoded
with anything else than UTF-8.
I would appreciate some input from experts who know more about mbstring and
encodings.

Regards,
Kamil

>