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

[WIP][HttpKernel] PSR6 Store for HttpCache #20061

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@jakzal
Copy link
Member

jakzal commented Sep 27, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR not yet

This is a WIP PR to get early feedback. I'd love to get this in for 3.2.

{
if (!$response->headers->has('X-Content-Digest')) {
$contentDigest = $this->generateContentDigest($response);
$item = $this->cacheBackend->getItem($contentDigest);

This comment has been minimized.

@iltar

iltar Sep 27, 2016

Contributor

I would make this a guard clause instead. Please note that it should be @return string|null

This comment has been minimized.

@jakzal

jakzal Sep 27, 2016

Member

Actually, I should create a CacheItem instead. No need to query the backend as we're going to overwrite the value anyway (if it's there).

use Symfony\Component\HttpFoundation\Response;
class Psr6Store implements StoreInterface
{

This comment has been minimized.

@iltar

iltar Sep 27, 2016

Contributor

Any reason to not make it final?

This comment has been minimized.

@jakzal

jakzal Sep 27, 2016

Member

Nope. Good idea.

/**
* @var CacheItemPoolInterface
*/
private $cacheBackend;

This comment has been minimized.

@ro0NL

This comment has been minimized.

@ro0NL

ro0NL Sep 27, 2016

Contributor

For being not in the Cache namespace, i'd favor $cachePool.

This comment has been minimized.

@jakzal

jakzal Sep 27, 2016

Member

Changed for consistency 👍

@jakzal jakzal force-pushed the jakzal:http-kernel/psr6-http-cache-store branch from d0ee060 to 1ed4431 Sep 27, 2016

@iltar

This comment has been minimized.

Copy link
Contributor

iltar commented Sep 27, 2016

@jakzal this ticket is probably explaining the causation? #4871

@jakzal

This comment has been minimized.

Copy link
Member

jakzal commented Sep 27, 2016

@iltar could be fixed by this PR indeed (expiration will be easier with backends like redis), but I was more thinking of making it more "small-website-prod-ready" (see #19705).

@jakzal jakzal force-pushed the jakzal:http-kernel/psr6-http-cache-store branch from 1ed4431 to 8aac80d Sep 27, 2016

@iltar

This comment has been minimized.

Copy link
Contributor

iltar commented Sep 27, 2016

@jakzal yeah saw that issue as well when looking for the one mentioned. While this is not the best to use for production servers, it's a nice way to get started if you don't (yet) have a proper caching layer. I think this is a nice feature to have.

@jakzal jakzal force-pushed the jakzal:http-kernel/psr6-http-cache-store branch 2 times, most recently from 7e548bb to 479b3f2 Sep 27, 2016

@jakzal jakzal force-pushed the jakzal:http-kernel/psr6-http-cache-store branch from 479b3f2 to 8a4c89b Sep 27, 2016

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

Looks like a good start :) Would it make sense to make an AbstractStorage to share a few things with other storages implems?

final class Psr6Store implements StoreInterface
{
/**
* @var CacheItemPoolInterface

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

not needed, already hinted in the constructor

private $cachePool;
/**
* List of locks acquired by the current process.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

not sure this adds value

/**
* List of locks acquired by the current process.
*
* @var array

This comment has been minimized.

@nicolas-grekas
private $locks = array();
/**
* @param CacheItemPoolInterface $cachePool

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

already in the constructor

}
/**
* Locates a cached Response for the Request provided.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

{@inheritdoc}

}
/**
* Writes a cache entry to the store for the given Request and Response.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

{@inheritdoc}

$headers = $response->headers->all();
unset($headers['age']);
$this->save($key, serialize(array(array($request->headers->all(), $headers))));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

serializing is already done by the cache pool

}
/**
* Invalidates all cache entries that match the request.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

{@inheritdoc}

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

(same below :) )

return true;
}
return $this->cachePool->hasItem($this->getLockKey($request));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

use $lockKey

*
* @return CacheItem
*/
private function createCacheItem($key, $value, $isHit = false)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2016

Member

This makes the implementation work only with Symfony's Cache components: cache items aren't interoperable.
You must get the item from the pool first (and maybe keep this as an optimization when Symfony Cache is used, another idea being to keep track of fetched items in this storage object to use them here)

This comment has been minimized.

@jakzal

jakzal Sep 28, 2016

Member

This is kind of feedback i wanted to get (ignore comments etc, i left them there for my reference until I'm done and replace them with inheritdoc) ;)

You must get the item from the pool first

I did it first, perhaps I tried to optimise too early and replaced a call to pool with this. I'll revert back. Once it's all working we can run tests and see if it needs improving.

This comment has been minimized.

@stof

stof Mar 31, 2017

Member

Having the optimization is OK if you keep the non-optimized version for other pools

@jakzal

This comment has been minimized.

Copy link
Member

jakzal commented Sep 28, 2016

Would it make sense to make an AbstractStorage to share a few things with other storages implems?

I thought about it, but the current implementation is very coupled to the filesystem. I'm currently mirroring the behaviour, but the current implementation is optimised for the filesystem. I expect the psr6 implementation diverge eventually to be optimised for the cache pool.

I'll try this approach nevertheless, to see which one makes more sense.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Sep 28, 2016

Just for the record, I'm very skeptical about this one. We explicitly tied the HTTP cache to the filesystem.

@jakzal

This comment has been minimized.

Copy link
Member

jakzal commented Sep 28, 2016

@fabpot no worries. If it's rejected here, I'll create a separate library (so work won't be wasted).

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 6, 2016

@Toflar

This comment has been minimized.

Copy link
Contributor

Toflar commented Mar 30, 2017

Now that there's a Lock component, do things get easier? @jakzal do you plan to continue the work on this?

@Toflar

This comment has been minimized.

Copy link
Contributor

Toflar commented Apr 7, 2017

FYI: I added a PR to the fork of @jakzal to push this further: jakzal#2
I was not sure whether I should just open a new PR because I actually like working together on something so I figured I'd just PR against the PR 😄

@Toflar

This comment has been minimized.

Copy link
Contributor

Toflar commented Apr 24, 2017

@jakzal any feedback here? Even if it's just a "sorry, no time, please open a new PR"? 😄

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jul 6, 2017

I'm closing this one as I think this is a bad idea for several reasons:

  • Using a filesystem allows for opcache to make the cache very effective;
  • The cache contains some PHP (when using ESI for instance) and storing PHP in anything else than a filesystem would mean eval()ing strings coming from Redis/Memcache/...,
  • HttpCache is triggered very early and does not have access to the container or anything else really. And it should stay that way to be efficient.

So, for these reasons, it won't be in Symfony core.

@fabpot fabpot closed this Jul 6, 2017

@Toflar Toflar referenced this pull request Sep 1, 2017

Closed

Symfony taggable cache #373

@jakzal jakzal deleted the jakzal:http-kernel/psr6-http-cache-store branch Sep 26, 2017

@Toflar

This comment has been minimized.

Copy link
Contributor

Toflar commented Dec 1, 2017

Leaving it here for people looking for a solution without it being in Symfony core: https://packagist.org/packages/toflar/psr6-symfony-http-cache-store

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Feb 8, 2018

Relying on PSR-6 has a nice side-effect: the ability to use cache tags (see api-platform/core#1689). Is supporting cache tags something we may want in core?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment