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

Filters are applied twice #134

Closed
danizord opened this issue Feb 13, 2017 · 5 comments
Closed

Filters are applied twice #134

danizord opened this issue Feb 13, 2017 · 5 comments

Comments

@danizord
Copy link
Contributor

danizord commented Feb 13, 2017

I have the following query param:

'load_relations' => [
    'location' => 'query',
    'type'     => 'string',
    'required' => false,
    'filters'  => ['json_encode'],
],

So, when I call $client->getCustomers(['load_relations' => ['Contact']); I'd expect Guzzle to send a request with the following query string: ?load_relations=%5B%22Contact%22%5D.
But instead I got: ?load_relations=%22%5B%5C%22Contact%5C%22%5D%22.

That's because Guzzle is filtering the value with json_encode twice:
1 - https://github.com/guzzle/guzzle-services/blob/1.1.0/src/Handler/ValidatedDescriptionHandler.php#L47
2- https://github.com/guzzle/guzzle-services/blob/1.1.0/src/RequestLocation/AbstractLocation.php#L65

danizord added a commit to zf-fr/zfr-lightspeed-retail that referenced this issue Feb 13, 2017
Actually we don't need a custom query serialization at all.
Lightspeed API is rejecting requests because of Guzzle double-filtering issue:
guzzle/guzzle-services#134
danizord referenced this issue May 30, 2017
Revert the commit 2b191b1 since it introduced a regression which affects the usage of operation extension.

See #145
@oliverde8
Copy link

Hi,

This issue is still present in the 1.1.2 release,

"startDate": {
          "description": "creation date for filtering. Format: 'yyyy-MM-dd'T'HH:mm:ss'",
          "location": "query",
          "sentAs": "start_date",
          "filters" : [
            {
              "method": "date_format",
              "args" : ["@value", "Y-m-d\\TH:i:s\\Z"]
            }
          ]
        },

This will crash because in the ValidationDescriptionHandler the filtering is already done, and later on, the filtering is done again by the Parameters->filter

which caused the fallowing issue

Warning: date_format() expects parameter 1 to be DateTimeInterface, string given  

I am not sure I understand why filtering is done at all at the validation step. Originally my parameter description was :

        "startDate": {
          "type": "object",
          "instanceOf" : "DateTime",
          "description": "creation date for filtering. Format: 'yyyy-MM-dd'T'HH:mm:ss'",
          "location": "query",
          "sentAs": "start_date",
          "filters" : [
            {
              "method": "date_format",
              "args" : ["@value", "Y-m-d\\TH:i:s\\Z"]
            }
          ]
        },

But this creates another error because the filtering is done before the check and therefore I don't have a \DateTime anymore`

@danizord
Copy link
Contributor Author

Hey @oliverde8, this issue was reintroduced since my PR #135 got reverted by 65ded51. It seems like #145 relies on this behavior :/

@Konafets
Copy link
Contributor

Hi,

the removal introduced a regression or as @danizord mentioned, some folks rely on the odd behavior. I plan to work on that issue and other stuff regarding this package in September again. Stay tuned or provide a PR which takes both issues into account and comes with proper tests :-)

@oliverde8
Copy link

oliverde8 commented Aug 20, 2017

Hi,

I will be using 1.1.1 for now as I have limited time on this part of the application and need to finish everything by mid September. I basically upgraded a library that was using guzzle 4, that I couldn't use due to other dependencies requiring guzzle 6.

If i have any time I will look into it for a PR, but I don't think I will have time,

Thanks for you support

@tlorens
Copy link

tlorens commented Apr 30, 2019

You can just create your own location class that extends AbstractLocation and override the functions that are calling the filters again.

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 a pull request may close this issue.

5 participants
@tlorens @Konafets @danizord @oliverde8 and others