Added a PSR6 cache plugin #15

Nyholm · 2015-11-27T11:45:25Z

Added a draft on a cache plugin. This can not be merged until PSR6 is final (currently voting).

require-dev psr/cache
write unit tests

dbu · 2015-11-27T12:21:01Z

src/CachePlugin.php

+     */
+    private function isCacheableResponse(ResponseInterface $response)
+    {
+        return floor($response->getStatusCode() / 100) === 2;


i think $statusCode >= 200 && $statusCode < 300 would be more readable. symfony is doing this:

https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/HttpFoundation/Response.php#L521-L529

i think we should respect cache control headers from the server.

dbu · 2015-11-27T12:23:54Z

looks cool! before we merge, i think you should add the psr cache to require-dev also and add some unit tests.

Nyholm · 2015-11-27T12:45:12Z

Thank you for the feedback. I agree and I will make some changes.

I have not added psr/cache to the require dev because it is not on packagist yet.

sagikazarmark · 2015-11-27T12:52:50Z

src/CachePlugin.php

+     * @param int                    $defaultTtl
+     * @param bool                   $respectCacheHeaders
+     */
+    public function __construct(CacheItemPoolInterface $pool, $defaultTtl = 60, $respectCacheHeaders = true)


Not sure, but shouldn't this be multiline? (PSR)

Only if more that 80 chars if I remember correctly.

+1 for a $options array with default_ttl and respect_cache_headers options for now. will be easier to scale.

sagikazarmark · 2015-11-27T13:00:00Z

Nice one @Nyholm, thanks.

A few more ideas:

Cache Control has Shared Max Age as well, although I am not sure we should care about it
Do we really need all those oneliners (checks mainly) in separate methods? Can they be reused?
Are there any possibility that we will have more options in the future? If yes, I think an options array in the constructor would be better.

dbu · 2015-11-27T13:39:31Z

I have not added psr/cache to the require dev because it is not on packagist yet.

ah sorry, makes sense :-) i will update the PR description with a tasklist to keep an overview

dbu · 2015-11-27T13:43:00Z

i would not use s-maxage i think. writing a proxy in PHP sounds like a weird idea. people could still extend the plugin if they found a use case for this and overwrite the getMaxAge method. or do a pull request for an option to use s-maxage instead of max-age.

Nyholm · 2015-11-27T13:44:37Z

Thank you for the feedback.

I have reduced the number of small one liner functions and improved the getCacheControlDirective.

About the Shared Max Age: I think (not sure) that shared max age should not be used by browser or clients. Only by reversed proxies.

About using array in the constructor: Is this forward compatible? Wouldn't a configuration object be better?

sagikazarmark · 2015-11-27T13:48:06Z

About using array in the constructor: Is this forward compatible? Wouldn't a configuration object be better?

I don't think we need a configuration object. An array can be extended dynamically without BC breaks, new options can be documented.

Not sure if we need it, just mentioned the possibility.

dbu · 2015-11-27T13:49:05Z

we could use the OptionsResolver from symfony would allow to validate the configuration. could be a nice compromise. a value object for configuration is a lot of effort and a bit awkward e.g. in symfony.

Nyholm · 2015-11-27T22:24:34Z

Okey, I have added some tests and make sure to use an array as second constructor argument to be more extendable in the future.
The tests will fail because of missing dependencies for psr/cache.

joelwurtz · 2015-11-28T11:09:26Z

What about etag and receving 304 Not modified ?

Nyholm · 2015-11-28T11:51:23Z

Do we want to support 304 or etag? If we do, we still have to make a HTTP request. The win is that we can get a small request back with a 304 header if the resource is "Not modified".

dbu · 2015-11-29T12:46:42Z

If-None-Match / If-Not-Modified headers would be really cool i think. for example a client doing some polling on a server that supports etags / last-modified headers.

i was thinking if we can add that separately, but i think its tightly coupled - you need the cached answer for the 304 to make any sense. the user of HTTPlug will still want to receive the answer. so ideally we just have an option if validation should be attempted - i guess it can default to true, its part of the HTTP spec.

in terms of caching, the tricky bit will be to keep answers cached for longer than max-age says, if they have an etag/last-modified header. basically we could keep them indefinitely. maybe we could have an optional config option for a maximum cache lifetime.

btw, note that if both last-modified and etag are present, BOTH have to be validated (in the sense of an AND, not OR).

Nyholm · 2015-12-07T20:28:48Z

I've updated the PR. The composer.json includes the git repository for psr/cache. Once the voting is complete we could use packagist instead.

joelwurtz · 2015-12-10T20:33:06Z

src/CachePlugin.php

+
+        if ($cacheItem->isHit()) {
+            // return cached response
+            return $cacheItem->get();


Hum, we save a response and so we return a reponse (when hitting the cache), but this method must return a promise, don't think it will work.

Should it not be a new FulfilledPromise($cacheItem->get()) ?

You are correct. Thank you for finding this issue!

Nyholm · 2015-12-10T20:51:11Z

Thank you for the feedback @joelwurtz. I've updated the PR now.

dbu · 2015-12-11T07:26:31Z

composer.json

+    "repositories": [
+        {
+            "type": "git",
+            "url":  "https://github.com/php-fig/cache.git"


just to not forget: we need to remove this before we merge.

Yes, PSR-6 should be tagged and added to packagist this week.

joelwurtz · 2015-12-17T11:40:40Z

PSR6 available on packagist :), so just need need a update of the composer and it's good to merge IMO

sagikazarmark · 2015-12-17T11:45:33Z

👍

Nyholm · 2015-12-17T12:06:25Z

I've updated the PR. I see that StyleCI fails but it not because of my changes.

I added #17 to fix the code style of the repo.

sagikazarmark · 2015-12-17T12:18:10Z

Merged it into master, can you rebase, so that build can run here as well?

Nyholm · 2015-12-17T12:28:58Z

Rebased!

sagikazarmark · 2015-12-17T12:40:15Z

@Nyholm thanks

Added a PSR-6 cache plugin

Nyholm · 2015-12-17T12:42:05Z

Thank you for all the feedback and comments

dbu reviewed Nov 27, 2015
View reviewed changes

sagikazarmark reviewed Nov 27, 2015
View reviewed changes

Nyholm force-pushed the psr6 branch from 9c6923f to d3ea36e Compare November 27, 2015 22:22

Nyholm force-pushed the psr6 branch from cea6121 to 8de36d3 Compare December 7, 2015 20:24

Nyholm changed the title ~~[WIP] Added a PSR6 cache plugin~~ Added a PSR6 cache plugin Dec 7, 2015

joelwurtz reviewed Dec 10, 2015
View reviewed changes

dbu reviewed Dec 11, 2015
View reviewed changes

Nyholm force-pushed the psr6 branch from f72e69e to 3939e6d Compare December 17, 2015 11:49

Nyholm force-pushed the psr6 branch 2 times, most recently from 8815e67 to b2f3434 Compare December 17, 2015 12:02

Added a PSR6 cache plugin

3d5870e

Nyholm force-pushed the psr6 branch from b2f3434 to 3d5870e Compare December 17, 2015 12:28

sagikazarmark added a commit that referenced this pull request Dec 17, 2015

Merge pull request #15 from Nyholm/psr6

b165dc3

Added a PSR-6 cache plugin

sagikazarmark merged commit b165dc3 into php-http:master Dec 17, 2015

Nyholm deleted the psr6 branch December 17, 2015 12:42

dbu mentioned this pull request Dec 17, 2015

Document Cache Plugin php-http/documentation#41

Closed

Added a PSR6 cache plugin #15

Added a PSR6 cache plugin #15

Uh oh!

Conversation

Nyholm commented Nov 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 27, 2015

Uh oh!

Nyholm commented Nov 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Nov 27, 2015

Uh oh!

dbu commented Nov 27, 2015 via email

Uh oh!

dbu commented Nov 27, 2015

Uh oh!

Nyholm commented Nov 27, 2015

Uh oh!

sagikazarmark commented Nov 27, 2015

Uh oh!

dbu commented Nov 27, 2015

Uh oh!

Nyholm commented Nov 27, 2015

Uh oh!

joelwurtz commented Nov 28, 2015

Uh oh!

Nyholm commented Nov 28, 2015

Uh oh!

dbu commented Nov 29, 2015

Uh oh!

Nyholm commented Dec 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Dec 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelwurtz commented Dec 17, 2015

Uh oh!

sagikazarmark commented Dec 17, 2015

Uh oh!

Nyholm commented Dec 17, 2015

Uh oh!

sagikazarmark commented Dec 17, 2015

Uh oh!

Nyholm commented Dec 17, 2015

Uh oh!

sagikazarmark commented Dec 17, 2015

Uh oh!

Nyholm commented Dec 17, 2015

Uh oh!

Uh oh!