Better support for querystrings (qs) #1437

sladg · 2023-03-03T16:11:53Z

PR based on #1383. Credits to @Nyholm.

This pull requests removed reliance on build-in parse_str function in favour of @otsch's https://github.com/crwlrsoft/query-string.

Allows to use dots and dashes in query parameters (won't covert them to underscores as parse_str does).

Fixes #756.

Makes #1042 and #1383 redundant.

Extra reading on dealing with parse_str:

sladg · 2023-03-03T16:14:30Z

@mnapoli I picked up on @Nyholm's work and switched to 3rd party solution. Let me know if this is up to the standards or some work is necessary on my end. Thanks!

Nyholm

Lovely.

Thank you. I got stuck with other things and it slipped my mind.
Im happy with the PR, I did two small comments.

composer.json

src/Event/Http/HttpRequestEvent.php

Nyholm

Thank you

sladg · 2023-03-04T18:06:43Z

Issue opened to address failing pipeline: crwlrsoft/query-string#4

otsch · 2023-03-23T08:33:19Z

@sladg Can I maybe help bringing it to Bref v2 as discussed in crwlrsoft/query-string#4?

mnapoli · 2023-03-23T11:04:25Z

I've changed the target for this PR to master (now that v2 is released).

@sladg could you rebase or merge master into your PR? That may make the build pass now.

sladg · 2023-03-23T11:16:51Z

@mnapoli got it! thought that the change did not work at first, anyways, let me rebase and I will be back in a second

…er names

sladg · 2023-03-23T11:25:45Z

@mnapoli rebased, we should be good to go :)

composer.json

GrahamCampbell · 2023-03-23T15:49:32Z

composer.json

@@ -4,7 +4,7 @@
    "type": "project",
    "description": "Bref is a framework to write and deploy serverless PHP applications on AWS Lambda.",
    "homepage": "https://bref.sh",
-    "keywords": ["bref", "serverless", "aws", "lambda", "faas"],
+    "keywords": [ "bref", "serverless", "aws", "lambda", "faas" ],


unrelated change shouldn't be included.

sladg · 2023-03-27T12:11:45Z

@mnapoli heya! checking if we can move forward with better qs support or there are some additional blockers :) thanks!

mnapoli · 2023-03-27T13:31:32Z

All good, thank you for this beautiful work to everyone involved!

aran112000 · 2023-04-12T11:55:49Z

Seems this PR introduced a bug for a rare edge case; if you have duplicate query string keys, only the final value will be passed through, these aren't treated as an array as expected.

Just submitted a PR to resolve this here:
crwlrsoft/query-string#5

otsch · 2023-04-12T13:55:58Z

Seems this PR introduced a bug for a rare edge case;

Correct me if I'm wrong, but I think actually it didn't introduce that behavior? I think it already behaved that way in previous (1.x) versions of bref.

I tried this, with both (v1.7 and v2.0):

include 'vendor/autoload.php';

use Bref\Event\Http\HttpRequestEvent;

$requestEvent = new HttpRequestEvent([
    'rawQueryString' => 'foo=bar&foo=baz',
    'httpMethod' => 'GET',
    'version' => '2.0',
]);

var_dump($requestEvent->getQueryParameters());

$requestEvent = new HttpRequestEvent([
    'requestContext' => ['elb' => true, 'http' => ['method' => 'GET']],
    'queryStringParameters' => [
        'foo' => 'bar',
        'foo' => 'baz',
    ],
]);

var_dump($requestEvent->getQueryParameters());

And in both cases the output is:

array(1) {
  ["foo"]=>
  string(3) "baz"
}
array(1) {
  ["foo"]=>
  string(3) "baz"
}

And looking at the bref source code, I think changing this behavior, would also require a change in bref, because:

Only when it's about an HTTP 2.0 request, the query params are actually initialized from the actual query string (foo=bar&foo=baz) https://github.com/brefphp/bref/blob/master/src/Event/Http/HttpRequestEvent.php#L219
In other situations, it is actually initialized from an array of query parameters like ['foo' => 'bar', 'foo' => 'baz'] https://github.com/brefphp/bref/blob/master/src/Event/Http/HttpRequestEvent.php#L230

So, in the second situation the incoming array would already have to be ['foo' => ['bar', 'baz']], because an array with the same key twice isn't possible.

Further, I think it'd best to keep the current behavior and just use a query string like foo[]=bar&foo[]=baz in those situations. Generally I think there is no strict rule what foo=bar&foo=baz should result in, server implementations can solve it as they want. But I think in most implementations (e.g. laravel) it results in ['foo' => 'baz'].
Is this OK for you @mnapoli @aran112000 ?

aran112000 · 2023-04-12T15:11:27Z

Correct me if I'm wrong, but I think actually it didn't introduce that behavior? I think it already behaved that way in previous (1.x) versions of bref.

Seems you're right 👍

And looking at the bref source code, I think changing this behavior, would also require a change in bref, because:

I don't believe that to be the case:

Correct, this does now work with API Gateway V2 with the changes in the PR (this is also the default for Bref these days I believe @mnapoli ?)
In other scenarios (ELB or API GW < 2), we have access to more in the Event payload (multiValueQueryStringParameters) which sorts this out from what I've seen in Bref and the tests coverage already there. I did start a PR for Bref earlier thinking that wasn't the case and closed it off after realising it already worked on older versions.

Further, I think it'd best to keep the current behaviour and just use a query string like foo[]=bar&foo[]=baz in those situations. Generally I think there is no strict rule what foo=bar&foo=baz should result in, server implementations can solve it as they want. But I think in most implementations (e.g. laravel) it results in ['foo' => 'baz'].

Completely agree the 'best' structure uses array notation[] on the parameters, but it's not uncommon to have query strings repeated. Even Google use it for Google Fonts for example when specifying multiple font families - for example:
https://fonts.googleapis.com/css2?family=Baloo&family=Roboto

otsch · 2023-04-12T17:25:22Z

Correct, this does now work with API Gateway V2 with the changes in the PR

Yes, with API Gateway V2 I guess it would work with the changes from the PR, but I don't think it's a good idea if it works differently in different environments.

In other scenarios (ELB or API GW < 2), we have access to more in the Event payload (multiValueQueryStringParameters) which sorts this out from what I've seen in Bref and the tests coverage already there. I did start a PR for Bref earlier thinking that wasn't the case and closed it off after realising it already worked on older versions.

OK, I just read a bit further on https://docs.aws.amazon.com/elasticloadbalancing/latest/application/lambda-functions.html#multi-value-headers and in the comments in bref's source. So, when multi-value is enabled, it reads like the multiValueQueryStringParameters already is an array like ['foo' => ['bar', 'baz']]. The crwlr/query-string is totally fine with that and wouldn't throw away any element, but actually bref converts it

first to an array like this: ['foo=bar', 'foo=baz'] in this foreach loop https://github.com/brefphp/bref/blob/master/src/Event/Http/HttpRequestEvent.php#L264
and then concatenates this array, back to a query string like foo=bar&foo=baz, which is then used to create a crwlr Query object instance from a string and use it to convert it to an array again 😅 (here https://github.com/brefphp/bref/blob/master/src/Event/Http/HttpRequestEvent.php#L272)

So, maybe the solution in this case is to just create the crwlr Query instance directly from the multiValueQueryStringParameters array, because it actually does the same back and forth conversion that bref does, inside the Query object. I'll try this and maybe send a PR.

otsch · 2023-04-12T17:49:44Z

Addition: I'll consider accepting your PR to the crwlr/query-string package @aran112000 👍🏻 have to take a closer look.

aran112000 · 2023-04-12T18:25:52Z

So, maybe the solution in this case is to just create the crwlr Query instance directly from the array, because it actually does the same back and forth conversion that bref does, inside the Query object. I'll try this and maybe send a PR.

I tried this route, API GW 2 doesn't provide multiValueQueryStringParameters, which I why I went this route

otsch · 2023-04-12T18:29:04Z

I tried this route, API GW 2 doesn't provide multiValueQueryStringParameters, which I why I went this route

Yes, that's why I then added, that I consider accepting your PR as well 👍🏻 I'm currently reviewing it...would you mind if I do some minor changes instead of requesting changes?

aran112000 · 2023-04-12T18:33:04Z

Gotcha, sure thing. If I can help at all, just shout

otsch · 2023-04-12T20:46:34Z

@mnapoli we changed the behavior in crwlr/query-string => crwlrsoft/query-string#5
In bref I know see that there's one test case failing with the new version of the package. It's FpmHandlerLoadBalancerTest::test alb query strings encoded with multi value enabled(). Is this bad?

otsch · 2023-04-12T21:17:18Z

@mnapoli OK, I found that the change introduced a new bug 🙈 already working on the fix.

otsch · 2023-04-12T21:36:43Z

@mnapoli OK, the tests are succeeding again with crwlr/query-string 1.0.3

mnapoli · 2023-04-13T08:59:33Z

Awesome, thanks a lot @otsch!

So anyone running composer update will get the fix I guess 👍

otsch · 2023-04-13T09:16:55Z

@mnapoli Yes 👍🏻 and as 1.0.2 only was the latest version for about an hour, not a lot (if any) should have gotten that version.

Support for getting array structure from duplicate query string keys without array syntax (brackets) was added in crwlr/query-string (v1.0.2) as requested in the discussion of this PR brefphp#1437. Example: foo=bar&foo=baz becomes ['foo' => ['bar', 'baz']] Add test case to make sure this actually works (and keeps working) in bref.

Nyholm approved these changes Mar 3, 2023

View reviewed changes

composer.json Outdated Show resolved Hide resolved

src/Event/Http/HttpRequestEvent.php Outdated Show resolved Hide resolved

Nyholm approved these changes Mar 3, 2023

View reviewed changes

sladg mentioned this pull request Mar 4, 2023

Compatibility with PHP 7 crwlrsoft/query-string#4

Closed

sladg changed the base branch from master to v2 March 23, 2023 11:03

mnapoli changed the base branch from v2 to master March 23, 2023 11:03

sladg changed the base branch from master to v2 March 23, 2023 11:14

sladg changed the base branch from v2 to master March 23, 2023 11:14

dmitchell9xb and others added 13 commits March 23, 2023 12:17

updating HttpRequestEvent query string rebuilding to preserve paramet…

a7bd58e

…er names

updating HttpRequestEvent query string rebuilding to preserve paramet…

9fc2feb

…er names

updating HttpRequestEvent query string rebuilding to preserve paramet…

11eefdc

…er names

Make sure we support query strings with period in them

ced0358

minor

7a39230

cs

780ad98

Added test from PR brefphp#1042

fecad67

Make sure tests dont change current behaviour

9dfc3a5

Removed licence text

d7d4eaa

cs

4aa6209

cleanup

d4cd3a3

ref(querystring): 3rd party package used for dealing with qs

8f3449e

style(http): return imidiately & revert change in scripts

d1c121b

sladg force-pushed the old-pr branch from fd7c292 to d1c121b Compare March 23, 2023 11:19

fix(composer): remove unused packages after rebase

3225eaa

sladg force-pushed the old-pr branch from 0164186 to 3225eaa Compare March 23, 2023 11:22

GrahamCampbell suggested changes Mar 23, 2023

View reviewed changes

composer.json Outdated Show resolved Hide resolved

fix(crwlr): package bumped to newer version due to breaking changes

4085984

GrahamCampbell suggested changes Mar 23, 2023

View reviewed changes

sladg requested a review from mnapoli March 27, 2023 12:11

mnapoli merged commit 2d78761 into brefphp:master Mar 27, 2023

This was referenced Mar 27, 2023

Rewrite query string parsing to avoid use parse_str #1042

Closed

Make sure we support query strings with period in them #1383

Closed

otsch mentioned this pull request Apr 13, 2023

Add test cases for query arrays without brackets #1509

Merged

sladg deleted the old-pr branch April 13, 2023 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better support for querystrings (qs) #1437

Better support for querystrings (qs) #1437

sladg commented Mar 3, 2023

sladg commented Mar 3, 2023

Nyholm left a comment •

edited

Nyholm left a comment

sladg commented Mar 4, 2023

otsch commented Mar 23, 2023

mnapoli commented Mar 23, 2023

sladg commented Mar 23, 2023

sladg commented Mar 23, 2023

GrahamCampbell Mar 23, 2023

sladg commented Mar 27, 2023

mnapoli commented Mar 27, 2023

aran112000 commented Apr 12, 2023

otsch commented Apr 12, 2023

aran112000 commented Apr 12, 2023 •

edited

otsch commented Apr 12, 2023

otsch commented Apr 12, 2023

aran112000 commented Apr 12, 2023

otsch commented Apr 12, 2023

aran112000 commented Apr 12, 2023

otsch commented Apr 12, 2023

otsch commented Apr 12, 2023

otsch commented Apr 12, 2023

mnapoli commented Apr 13, 2023

otsch commented Apr 13, 2023

Better support for querystrings (qs) #1437

Better support for querystrings (qs) #1437

Conversation

sladg commented Mar 3, 2023

sladg commented Mar 3, 2023

Nyholm left a comment • edited

Choose a reason for hiding this comment

Nyholm left a comment

Choose a reason for hiding this comment

sladg commented Mar 4, 2023

otsch commented Mar 23, 2023

mnapoli commented Mar 23, 2023

sladg commented Mar 23, 2023

sladg commented Mar 23, 2023

GrahamCampbell Mar 23, 2023

Choose a reason for hiding this comment

sladg commented Mar 27, 2023

mnapoli commented Mar 27, 2023

aran112000 commented Apr 12, 2023

otsch commented Apr 12, 2023

aran112000 commented Apr 12, 2023 • edited

otsch commented Apr 12, 2023

otsch commented Apr 12, 2023

aran112000 commented Apr 12, 2023

otsch commented Apr 12, 2023

aran112000 commented Apr 12, 2023

otsch commented Apr 12, 2023

otsch commented Apr 12, 2023

otsch commented Apr 12, 2023

mnapoli commented Apr 13, 2023

otsch commented Apr 13, 2023

Nyholm left a comment •

edited

aran112000 commented Apr 12, 2023 •

edited