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] [2.2] [HttpKernel] added commands to clear, cleanup and introspect the HttpCache #6213

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@lsmith77
Contributor

lsmith77 commented Dec 6, 2012

Q A
Bug fix? no
New feature? yes
BC breaks? yes (see below)
Deprecations? -
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

this is a bit hacky since its not so easy to get the HttpCache instance for the project. essentially the solution is that it is expected that the kernel class is autoloadable (and that it can be instanciated by simply passing the actual kernel as a constructor argument).

i also made some adjustments to the Store interface to be able to give more information about cache entries but to prevent BC breaks maybe this should be done in a new interface.

i am not sure if some of the logic in the commands should sit in the HttpKernel component and then extended in the Bundle, but that would make it harder to keep the redundant logic in an abstract class.

furthermore i wonder if there is a sensible way to make this pluggable with other reverse proxies. i have a simple solution for varnish purging here: https://github.com/liip/LiipCacheControlBundle/blob/master/Helper/Varnish.php

ezPublish guys have something more sophisticated here https://github.com/ezsystems/ezp-next/tree/master/eZ/Publish/Core/MVC/Symfony/Cache/Http

@beberlei

This comment has been minimized.

Contributor

beberlei commented Dec 6, 2012

There is room for an abstract class that handles the code duplication of the three commands.

@vicb

This comment has been minimized.

Contributor

vicb commented Dec 6, 2012

@lsmith77 👍 for the idea (didn't check the imp). A few things I would like to see adressed (not sure if they are already part of this PR):

  • Clear the outdated cache items,
  • Clear by path only (ie You should not have to specify all the parameters).
@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Dec 6, 2012

there is a commend that calls Store::cleanup(), however this method currently only unlocks files that where locked in the same request. so the code path that calls it is semi useless, but yeah i figured that someone might eventually add the ability to clean out stale data. clearing by path is supported.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Dec 7, 2012

@beberlei i have created an abstract base class

@stof

View changes

src/Symfony/Bundle/FrameworkBundle/Command/AbstractHttpCacheCommand.php Outdated
* @return \Symfony\Component\HttpKernel\HttpCache\HttpCache
* @throws \RuntimeException
*/
protected function getCacheKernel(InputInterface $input, Kernel $kernel)

This comment has been minimized.

@stof

stof Dec 7, 2012

Member

This typehint should be KernelInterface

@@ -106,6 +106,11 @@ public function isLocked(Request $request)
return is_file($this->getPath($this->getCacheKey($request).'.lck'));
}
public function isCached(Request $request)

This comment has been minimized.

@lsmith77

lsmith77 Dec 7, 2012

Contributor

should maybe be moved to a different interface?

@@ -361,6 +366,11 @@ private function save($key, $data)
@chmod($path, 0666 & ~umask());
}
public function getLocation(Request $request)

This comment has been minimized.

@lsmith77

lsmith77 Dec 7, 2012

Contributor

should maybe be moved to a different interface?

use Symfony\Component\HttpFoundation\Request;
/**
* TODO: this command is a bit all over the place

This comment has been minimized.

@lsmith77

lsmith77 Dec 7, 2012

Contributor

this command needs more work

@vicb

This comment has been minimized.

Contributor

vicb commented Dec 7, 2012

I think I need to clarify the following:

Clear by path only (ie You should not have to specify all the parameters)

When you render a fragment, the parameters are passed via the query string. _internal/secure/show/name?name=lsmith, I really don't want to call clear cache for every user in my db, I just want to be able to purge _internal/secure/show/name.

/cc @lsmith77 @kbond

ref: https://twitter.com/zenstruck/status/277028556985470976

@kbond

This comment has been minimized.

Contributor

kbond commented Dec 7, 2012

Right, but I can see a use case where you would want to purge the cache for a path, including all the query strings:

Imagine you have these urls all cached:

  • /products/list
  • /products/list?sort=desc
  • /products/list?sort=asc

You might want to purge all three with one purge call.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Dec 7, 2012

ah yes .. for that use case we would need to rework the index quite a bit. however we could also consider to simply add an optional secondary database and come up with a generic solution to not only index by path, but also by header etc.

ezPublish devs did something for a specific header here:
https://github.com/ezsystems/ezp-next/blob/master/eZ/Publish/Core/MVC/Symfony/Cache/Http/LocationAwareStore.php

cc @lolautruche

@vicb

This comment has been minimized.

Contributor

vicb commented Dec 7, 2012

@lsmith77 we should keep in mind that HttpCache is mainly for dev purpose. Please don't come up with a php-varnish.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Dec 7, 2012

HttpCache is not for dev purposes. if you want to deploy to Varnish, then you have to use Varnish in development. HttpCache is for smaller projects where setting up and maintaining VLC scripts its overkill.

@kbond

This comment has been minimized.

Contributor

kbond commented Dec 7, 2012

I agree, I use HttpCache for several small to medium projects and have no intention to switch any of them to varnish.

@vicb

This comment has been minimized.

Contributor

vicb commented Dec 7, 2012

@lsmith77 @kbond I can understand your POV. It would also mean that this PR is premature and the HttpCache refactoring has to be done first, correct ?

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Dec 7, 2012

well refactoring or adding features to HttpCache is indeed a separate topic. as you see i have made some minor changes already which i could revert. but this is not to say that its not possible to offer something viable and useful even with the current state of HttpCache.

i just dont want these commands to be dropped just because we dont get around to refactoring HttpCache

@dlsniper

This comment has been minimized.

Contributor

dlsniper commented Dec 11, 2012

I might have some time to help out this weekend but I'm not sure about it if no one else is able to help until then.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Dec 25, 2012

i guess a lot of what i would like to achieve should best be provided via a generic caching layer that supports tagging.

@lsmith77 lsmith77 referenced this pull request Jan 4, 2013

Merged

[WIP] Kernel refactor #6459

fabpot added a commit that referenced this pull request Jan 11, 2013

merged branch fabpot/kernel-refactor (PR #6459)
This PR was merged into the master branch.

Commits
-------

76fefe3 updated CHANGELOG and UPGRADE files
f7da1f0 added some unit tests (and fixed some bugs)
f17f586 moved the container aware HTTP kernel to the HttpKernel component
2eea768 moved the deprecation logic calls outside the new HttpContentRenderer class
bd102c5 made the content renderer work even when ESI is disabled or when no templating engine is available (the latter being mostly useful when testing)
a8ea4e4 [FrameworkBundle] deprecated HttpKernel::forward() (it is only used once now and not part of any interface anyway)
1240690 [HttpKernel] made the strategy a regular parameter in HttpContentRenderer::render()
adc067e [FrameworkBundle] made some services private
1f1392d [HttpKernel] simplified and enhanced code managing the hinclude strategy
403bb06 [HttpKernel] added missing phpdoc and tweaked existing ones
892f00f [HttpKernel] added a URL signer mechanism for hincludes
a0c49c3 [TwigBridge] added a render_* function to ease usage of custom rendering strategies
9aaceb1 moved the logic from HttpKernel in FrameworkBundle to the HttpKernel component

Discussion
----------

[WIP] Kernel refactor

Currently, the handling of sub-requests (including ESI and hinclude) is mostly done in FrameworkBundle. It makes these important features harder to implement for people using only HttpKernel (like Drupal and Silex for instance).

This PR moves the code to HttpKernel instead. The code has also been refactored to allow easier integration of other rendering strategies (refs #6108).

The internal route has been re-introduced but it can only be used for trusted IPs (so for the internal rendering which is managed by Symfony itself, or by a trusted reverse proxy like Varnish for ESI handling). For the hinclude strategy, when using a controller, the URL is automatically signed (see #6463).

The usage of a listener instead of a controller to handle internal sub-requests speeds up things quite a lot as it saves one sub-request handling. In Symfony 2.0 and 2.1, the handling of a sub-request actually creates two sub-requests.

Rendering a sub-request from a controller can be done with the following code:

```jinja
{# default strategy #}
{{ render(path("partial")) }}
{{ render(controller("SomeBundle:Controller:partial")) }}

{# ESI strategy #}
{{ render(path("partial"), { strategy: 'esi' }) }}
{{ render(controller("SomeBundle:Controller:partial"), { strategy: 'esi' }) }}

{# hinclude strategy #}
{{ render(path("default1"), { strategy: 'hinclude' }) }}
```

The second commit allows to simplify the calls a little bit thanks to some nice syntactic sugar:

```jinja
{# default strategy #}
{{ render(path("partial")) }}
{{ render(controller("SomeBundle:Controller:partial")) }}

{# ESI strategy #}
{{ render_esi(path("partial")) }}
{{ render_esi(controller("SomeBundle:Controller:partial")) }}

{# hinclude strategy #}
{{ render_hinclude(path("default1")) }}
```

---------------------------------------------------------------------------

by fabpot at 2013-01-03T17:58:49Z

I've just pushed a new version of the code that actually works in my browser (but I've not yet written any unit tests). I've updated the PR description accordingly.

All comments welcome!

---------------------------------------------------------------------------

by Koc at 2013-01-03T20:11:43Z

what about `render(controller="SomeBundle:Controller:partial", strategy="esi")`?

---------------------------------------------------------------------------

by stof at 2013-01-04T09:01:01Z

shouldn't we have interfaces for the UriSigner and the HttpContentRenderer ?

---------------------------------------------------------------------------

by lsmith77 at 2013-01-04T19:28:09Z

btw .. as mentioned in #6213 i think it would make sense to refactor the HttpCache to use a cache layer to allow more flexibility in where to cache the data (including clustering) and better invalidation. as such if you are refactoring HttpKernel .. it might also make sense to explore splitting off HttpCache.

---------------------------------------------------------------------------

by fabpot at 2013-01-04T19:30:07Z

@lsmith77 This is a totally different topic. This PR is just about moving things from FrameworkBundle to HttpKernel to make them more reusable outside of the full-stack framework.

---------------------------------------------------------------------------

by fabpot at 2013-01-05T09:39:52Z

I think this PR is almost ready now. I still need to update the docs and add some unit tests. Any other comments on the whole approach? The class names? The `controller` function thingy? The URI signer mechanism? The proxy protection for the internal controller? The proxy to handle internal routes?

---------------------------------------------------------------------------

by sstok at 2013-01-05T10:08:25Z

Looks good to me 👍

---------------------------------------------------------------------------

by sdboyer at 2013-01-07T18:17:08Z

@Crell asked me to weigh in, since i'm one of the Drupal folks who's likely to work most with this.

i think i've grokked about 60% of the big picture here, and i'm generally happy with what i see. the assumption that the HInclude strategy makes about working with templates probably isn't one that we'll be able to use (and so, would need to write our own), but that's not a big deal since the whole goal here is to make strategies pluggable.

so, yeah. +1.

---------------------------------------------------------------------------

by winzou at 2013-01-09T20:21:44Z

Just for my information: will this PR be merged for 2.2 version? Thanks.

---------------------------------------------------------------------------

by stof at 2013-01-09T20:41:04Z

@winzou according to the blog post announcing the beta 1 release, yes. It is explicitly listed as being one of the reason to make it a beta instead of the first RC.

---------------------------------------------------------------------------

by winzou at 2013-01-09T20:49:36Z

OK thanks, I've totally skipped this blog post.

---------------------------------------------------------------------------

by fabpot at 2013-01-10T15:26:15Z

I've just added a bunch of unit tests and fix some bugs I found while writing the tests.
@fabpot

This comment has been minimized.

Member

fabpot commented Jun 13, 2016

Closing this very old PR as it has been stalled for so many years and in WIP mode.

@fabpot fabpot closed this Jun 13, 2016

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