Adding response mutator. #48

iainconnor · 2018-01-15T17:43:43Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #X, partially #Y, mentioned in #Z
Documentation	php-http/documentation#221
License	MIT

What's in this PR?

Adds an optional mutator callback so you can mutate the response depending whether it was from cache or not.

Why?

Right now, there's no way to tell whether a response came from the cache or not, other than reverse-engineering the cache logic. It may be useful for debugging or tweaking your cache to know this, so this PR provides an optional way of retrieving this information. An AddHeaderResponseMutator is provided as an example way of adding a custom header to the response identifying whether it came from cache or not.

Example Usage

CachePlugin::clientCache($cacheItemPool, $streamFactory, ['response_mutator' => new AddHeaderResponseMutator('X-From-Cache')]);

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Documentation pull request created (if not simply a bugfix)

To Do

Not sure how to update CHANGELOG.md. Should I be tagging a new version?

Nyholm

This question has came up before. I do not really see the value of it. But this is a good implementation and I do not see why we should not allow it.

Nyholm · 2018-01-15T17:51:25Z

src/CachePlugin.php

@@ -357,6 +367,7 @@ private function configureOptions(OptionsResolver $resolver)

            return empty($matches);
        });
+        $resolver->setAllowedTypes('response_mutator', ['null', 'Http\Client\Common\Plugin\Cache\Mutator\ResponseMutator']);


We could use the ::class constant

I agree, but I was following the style of the other lines in this method.

lets use it here and we fix it elsewhere.

(but as saying below, i think this should be an array, while we are at it.

Nyholm · 2018-01-15T17:52:36Z

src/Mutator/AddHeaderResponseMutator.php

+    /**
+     * @param string $headerName
+     */
+    public function __construct($headerName)


How about adding a default value here? I guess most people will be happy with any default.

Agreed, I'll update with a sane default.

dbu

thanks for this one! the architecture looks clean to me.

a way to achieve this with existing code features would be to put a plugin into the chain after the cache, and one before the cache. the after adds the MISS value, the one before checks if X-Cache is set and if not, its a HIT. but i agree that that would be quite an incidental way of doing things.

are there any uses cases other than adding an X-Cache: HIT/MISS header? i think either we should find reasons why additional flexibility is needed, or simply have a flag (or header name config) to make the cache plugin itself add the hit or miss header. other plugins could then look at that header to know if they need to further change the response, if you need to change a response based on whether it comes from the cache. if such a scenario exists at all... maybe its better to just have the header, and if somebody has a really special need, they can always implement their own cache plugin based on this repo?

dbu · 2018-01-16T07:24:09Z

src/CachePlugin.php

+    private function handleResponseMutator(RequestInterface $request, ResponseInterface $response, $cacheHit)
+    {
+        if (null !== $this->config['response_mutator']) {
+            $response = $this->config['response_mutator']->mutate($request, $response, $cacheHit);


i think this should be a list of mutators.

Good suggestion, will update.

Though, not totally familliar with the resolver... is there any better way of stating that the array must be of generic type CacheListener? Right now I just have $resolver->setAllowedTypes('cache_listeners', ['array']);.

dbu · 2018-01-16T07:27:07Z

src/CachePlugin.php

@@ -357,6 +367,7 @@ private function configureOptions(OptionsResolver $resolver)

            return empty($matches);
        });
+        $resolver->setAllowedTypes('response_mutator', ['null', 'Http\Client\Common\Plugin\Cache\Mutator\ResponseMutator']);


lets use it here and we fix it elsewhere.

(but as saying below, i think this should be an array, while we are at it.

dbu · 2018-01-16T07:28:08Z

src/Cache/Mutator/AddHeaderResponseMutator.php

+    /**
+     * @param string $headerName
+     */
+    public function __construct($headerName = 'X-From-Php-Http-Cache')


a semi-official way to mark cache hits is to use "X-Cache: HIT" (resp. MISS). can we find a different example?

Good suggestion, updated.

dbu · 2018-01-16T07:28:52Z

src/Cache/Mutator/AddHeaderResponseMutator.php

+     *
+     * @return string
+     */
+    public function mutate(RequestInterface $request, ResponseInterface $response, $cacheHit)


is there any more information that we could pass in instead of the boolean? like the $cacheItem or such? then this could be used to extract additional debugging information if wanted.

Good suggestion, updated.

For the record, I'm going to keep the cacheHit parameter as well, so the consumer doesn't have to reverse-engineer the logic regarding expiry. Checking cacheItem->isHit() won't be sufficient.

dbu · 2018-01-16T07:31:57Z

src/Cache/Mutator/ResponseMutator.php

+ *
+ * @author Iain Connor <[email protected]>
+ */
+interface ResponseMutator


i am not happy with this name. every plugin has the possibility to change a response. this thing is specifically about the cache. but unfortunately i don't have a great name either. CacheListener is not too good, but would be more specific to what this does. can we be even more specific?

the class doc could be "XXX is called by the cache plugin with information on the cache status to change information on the response based on whether it was a cache hit or a miss."

I can't think of anything significantly better. Will rename this for the time being and see how it feels.

iainconnor · 2018-01-16T14:56:14Z

are there any uses cases other than adding an X-Cache: HIT/MISS header?

The other use-case that my project is currently adding is logging of the hit/miss rate. I realize I could probably accomplish that in another way (by adding the header and then checking for the presence of that header, for example), but adding it the same plugin seemed clearer.

dbu · 2018-01-19T09:07:31Z

@Nyholm want another look?

also, should we support this in the symfony bundle too?

ResponseMutator documentation for php-http/cache-plugin#48

iainconnor · 2018-01-29T15:17:58Z

Saw the documentation get merged in. Anything else needed for this code?

dbu · 2018-01-29T15:26:12Z

ups, somehow missed to merge this. the changelog is still open. can you please update composer.json to make the branch alias 1.6 and add a changelog entry for 1.6.0 that says this was added (short, but say what its for)?

iainconnor · 2018-01-29T15:33:55Z

Sure thing; done.

dbu · 2018-01-29T16:52:57Z

yay, thanks a lot!

iainconnor added 2 commits January 15, 2018 11:30

Adding response mutator.

a0192c0

Fixing style.

8abb28a

Nyholm approved these changes Jan 15, 2018

View reviewed changes

iainconnor added 3 commits January 15, 2018 11:55

Adding default value to header.

63f438f

Adding request to callback.

465c0e4

Moving directory

f62329e

iainconnor mentioned this pull request Jan 16, 2018

Adding documentation for ResponseMutator. php-http/documentation#221

Merged

dbu reviewed Jan 16, 2018

View reviewed changes

iainconnor added 11 commits January 16, 2018 08:56

Renaming default header and values.

a968e8f

Pass through cache item as well.

884c87c

Adding support for multiple listeners and renaming.

0e5159f

Whoops wrong class.

fef55cf

Whoops wrong class.

e1e4bec

Finishing rename.

c29b205

Respecting code style.

3edb6df

bad docblock

ce5224d

Must be nullable.

27e053b

Bad return type.

c5e2ac2

Wrong var name.

d0f2494

dbu approved these changes Jan 19, 2018

View reviewed changes

dbu pushed a commit to php-http/documentation that referenced this pull request Jan 22, 2018

Adding documentation for ResponseMutator. (#221)

ca229f4

ResponseMutator documentation for php-http/cache-plugin#48

Adding changelog and updating version in composer.

699d45e

Nyholm approved these changes Jan 29, 2018

View reviewed changes

dbu merged commit 5ae8a80 into php-http:master Jan 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding response mutator. #48

Adding response mutator. #48

iainconnor commented Jan 15, 2018 •

edited

Loading

Nyholm left a comment

Nyholm Jan 15, 2018

iainconnor Jan 15, 2018

dbu Jan 16, 2018

Nyholm Jan 15, 2018

iainconnor Jan 15, 2018

dbu left a comment

dbu Jan 16, 2018

iainconnor Jan 16, 2018

iainconnor Jan 16, 2018

dbu Jan 16, 2018

dbu Jan 16, 2018

iainconnor Jan 16, 2018

dbu Jan 16, 2018

iainconnor Jan 16, 2018

iainconnor Jan 16, 2018

dbu Jan 16, 2018

iainconnor Jan 16, 2018

iainconnor commented Jan 16, 2018

dbu commented Jan 19, 2018

iainconnor commented Jan 29, 2018

dbu commented Jan 29, 2018

iainconnor commented Jan 29, 2018

dbu commented Jan 29, 2018

Adding response mutator. #48

Adding response mutator. #48

Conversation

iainconnor commented Jan 15, 2018 • edited Loading

What's in this PR?

Why?

Example Usage

Checklist

To Do

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainconnor commented Jan 16, 2018

dbu commented Jan 19, 2018

iainconnor commented Jan 29, 2018

dbu commented Jan 29, 2018

iainconnor commented Jan 29, 2018

dbu commented Jan 29, 2018

iainconnor commented Jan 15, 2018 •

edited

Loading