csarrazi / CsaGuzzleBundle

A bundle integrating Guzzle >=4.0 in Symfony
250 stars 76 forks source link

Mock filename tokenizer #208

Closed Ciloe closed 6 years ago

Ciloe commented 6 years ago

Hi, I saw a problem with the fixture file name generated by this function :

src/Cache/NamingStrategy/AbstractNamingStrategy.php

protected function getFingerprint(RequestInterface $request)
    {
        return md5(serialize([
            'method' => $request->getMethod(),
            'path' => $request->getUri()->getPath(),
            'query' => $request->getUri()->getQuery(),
            'user_info' => $request->getUri()->getUserInfo(),
            'port' => $request->getUri()->getPort(),
            'scheme' => $request->getUri()->getScheme(),
            'headers' => array_diff_key($request->getHeaders(), array_flip($this->blacklist)),
        ]));
    }

It's appear that if wee requested a graphQl API, we use the body to send the request and not the query. Is it possible to add 'body' => $request->getBody(), in this array ?

Thank you

csarrazi commented 6 years ago

Thanks for the suggestion and the PR!

However, I think we should do something more intelligent in this case, as GraphQL breaks REST principles.

The main difference between REST and GraphQL is that the body should have no influence on the result of the request for the former, whereas it is the case for the latter.

Instead, in order to add support for GraphQL, let’s create a dedicated naming strategy.

Ciloe commented 6 years ago

Yes, I think a new naming strategy is a good idea. But what can we use in the strategy ? I think this header is the minimum

       [
            'method' => $request->getMethod(),
            'user_info' => $request->getUri()->getUserInfo(),
            'port' => $request->getUri()->getPort(),
            'scheme' => $request->getUri()->getScheme(),
            'headers' => array_diff_key($request->getHeaders(), array_flip($this->blacklist)),
        ]

But, with my tests I can see some problems.

What is the good solution for you ?

csarrazi commented 6 years ago

Hi @Ciloe, as I have split the bundle's middleware in multiple libraries, let us continue the discussion in csarrazi/guzzle-cache-middleware :)