EZEE-1780: Plugin support by vidarl · Pull Request #19 · ezsystems/ezplatform-http-cache

vidarl · 2017-11-02T11:58:34Z

Issue: https://jira.ez.no/browse/EZEE-1780

This PR makes it possible to extend http cache in 3rd party bundles

Depends on:

~~EZP-28243: Purge type using service id does not work ezpublish-kernel#2136~~ (merged)

vidarl · 2017-11-02T12:07:38Z

src/Resources/config/view_cache.yml

+    ezplatform.view_cache.response_tagger.restlocationbc:
+        class: EzSystems\PlatformHttpCacheBundle\ResponseTagger\Value\RestLocationBCTagger
+        tags:
+            - {name: ezplatform.cache_response_tagger}


@bdunogier :
I really don't get the reasoning behind splitting the response taggers in delegators and values. It just creates overhead? Also, given that we keep this split, why does also the value tagges have the ezplatform.cache_response_tagger tag? It means they are invoked both by the dispatcher directly, and by their deligator

andrerom · 2017-11-05T13:39:12Z

src/EventSubscriber/HttpCacheResponseSubscriber.php

-        $view = $event->getRequest()->attributes->get('view');
-        if (!$view instanceof CachableView || !$view->isCacheEnabled()) {
-            return;
+        if ($event->getRequest()->attributes->has('is_rest_request') && $event->getRequest()->attributes->get('is_rest_request') === true) {


Maybe this subscriber needs a new name, but this subscriber to me is for ContentView.

Not all rest requests are supposed to be cacheable afaik, so if (we make it) expose RestValue or something on Response (is it maybe still on the getControllerResult?), then we can have a dedicated subscriber for rest responses.

see #25 I would think this part is not needed anymore if we handle X-Location-Id issue in separate listener

+1 on @andrerom comment

andrerom · 2017-11-05T13:41:24Z

src/ResponseTagger/Delegator/RestXLocationBCTagger.php

+    {
+        // Rest API sets the X-Location-Id header. This needs to be changed according to http cache in use
+        if ($value === null && $response->headers->has('X-Location-Id')) {
+            $this->restLocationBCTagger->tag($configurator, $response, ['X-Location-Id' => $response->headers->get('X-Location-Id')]);


Kind of duplicates #21

And it seems a bit strange to use several layers of response taggers to map X-Location-id to xkey and remove it.

and now it is also useless because of #25 I think

Plopix · 2017-11-05T17:38:35Z

src/PurgeClient/VarnishPurgeClient.php

@@ -14,7 +14,7 @@
 * Only support BAN requests on purpose, to be able to invalidate cache for a


BAN not updated right?

yeah, that was an old comment... Removed it

Plopix · 2017-11-05T18:02:29Z

src/DependencyInjection/Compiler/PurgeTypeSettingsPass.php

+        // eZ\Publish\Core\MVC\Symfony\Cache\Http\FOSPurgeClient
+        // Or else, that deprecated class is still used by cache:clear
+        if ($purgeType !== 'local') {
+            $container->setAlias('ezpublish.http_cache.purge_client', $purgeServiceId);


should use ezplatform.http_cache.purge_client I think because this bundle is overriding it
See other bug

No, I meant what I wrote here....
This line ensured that correct purge client was used when running php app/console clear:cache

However, it seems like it was decided in #15 that we should not clear http cache when doing a clear cache

So, if #15 stays, this line can be removed

15 was not necessarily the final say on it, it was to solve a bug where kernel purge system will be called on cache:clear, when if we need such logic should have the proper config for it here.

But we don't clear Stash / Symfony cache either on cache:clear by default (unless file system cache is used), but it should be possible somehow to do it when content or schema for some reason changes on deployment. If so, purge for symfony cache, and expire (soft purge) for http cache.

But this line can indeed be removed, ca:cl is not supposed to call kernel purge client anymore with this bundle enabled. Actually almost all http cache code in kernel is deprecated in favor of this bundle.

in any case are we sure we don't want to use ezplatform.http_cache.purge_client and not ezpublish.http_cache.purge_client one is meant to replace the other right ?

I don't think I understood your last comment, but I have removed the line in question as it does not do anything after #15 was merged

Plopix · 2017-11-05T18:05:23Z

src/DependencyInjection/Compiler/PurgeTypeSettingsPass.php

+{
+    public function process(ContainerBuilder $container)
+    {
+        $purgeType = $container->getParameter('purge_type');


we should not use the parameter here... that could be renamed. we should use the ezpublish.http_cache.purge_type settings

Hmm.. Not sure that is possible in a compiler pass. You know how?

Not by default, I think, no way :( But for sure we should do that on purge_type.

You will have to set the value as a parameter here:
https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Bundle/EzPublishCoreBundle/DependencyInjection/EzPublishCoreExtension.php#L392

$container->setParameter("ezpublish.http_cache.purge_type", $config['http_cache']['purge_type']);

And then refer to it

$purgeType = $container->getParameter('ezpublish.http_cache.purge_type');

Then comes the SiteAccess Awareness, but I think that is global then a parameter would be fine.

@andrerom @bdunogier another idea?

Right, fixed as suggested

This reverts commit 6b1dafa.

andrerom · 2017-11-15T11:13:15Z

src/PurgeClient/PurgeClientInterface.php

-     * Triggers the cache purge $locationIds.
+     * Triggers the cache purge of $tags.
     *
     * It's up to the implementor to decide whether to purge $locationIds right away or to delegate to a separate process.


$locationIds

So you insist on calling it $locationIds ?
It is not consistent with https://github.com/ezsystems/ezplatform-http-cache/pull/19/files#diff-cdb40d122691729c63752db68cddf3f9R31 and other places
And besides, there are not only location ids, also path-XX, content-XX and content-type-XX etc.....

No, you misunderstand me. I was referring to text here mentioning $locationIds when it should be aligned with the other changes you make to refer to tags instead.

ah... missed that one.. will fix

andrerom

+1, but I guess this depends on kernel changes

so:

I'll ping you on that separately
Requirement for kernel should then be bumped here in this PR to:
"ezsystems/ezpublish-kernel": "~6.7.7@dev || ^6.12.1@dev || ^7.0@dev"

DominikaK

A few language nitpicks in the doc.

DominikaK · 2017-11-15T14:30:02Z

docs/drivers.md

+ - TagHandler
+ - FOS TagHandler
+
+If you write a new PurgeClient driver, you **must** also create a corresponding TagHandler and vise


...TagHandler and vice

DominikaK · 2017-11-15T14:30:52Z

docs/drivers.md

+
+The PurgeClient is responsible for sending purge requests to the http cache when content is about to be invalidated.
+The PurgeClient must implement EzSystems\PlatformHttpCacheBundle\PurgeClient\PurgeClientInterface and can be registered
+with the following code in service.yml:


Isn't it serviceS.yml?

yes, it is. good catch....

DominikaK · 2017-11-15T14:35:36Z

docs/drivers.md

+
+## FOS TagHandler
+
+The FOS Http cache bundle also have a TagHandler which is not used by eZ Platform except for one thing, the


...also has a TagHandler...

DominikaK · 2017-11-15T14:36:04Z

docs/drivers.md

+## FOS TagHandler
+
+The FOS Http cache bundle also have a TagHandler which is not used by eZ Platform except for one thing, the
+`fos:httpcache:invalidate:tag` command. With this command you may explicit invalidate cache by tag.


...you may explicitly invalidate cache...

DominikaK · 2017-11-15T14:38:02Z

docs/drivers.md

+
+Normally, you would not need to implement your own FOS TagHandler as the ezplatform-http-cache bundle ships with a
+default one which uses the PurgeClient to invalidate the given tags.
+If you anyway need to write your own FOS TagHandler, you may register it with the following code in service.yml:


If you need to write your own FOS TagHandler anyway...

andrerom · 2017-11-16T09:41:10Z

src/DependencyInjection/EzSystemsPlatformHttpCacheExtension.php

+
+    public function getAlias()
+    {
+        return 'ez_platform_cache';


ez_platform_http_cache, method can afaik be avoided by renaming this class to EzPlatformHttpCacheExtension which getContainerExtension() will then need to be adapted for.

I don't think that will be better. If I rename class, then I think the most clean way should be to also override Bundle::getContainerExtensionClass() ( and not write logic concerning class name in getContainerExtension() ).

So IMO it is either to create a getAlias() or a getContainerExtensionClass() method.

Replaced getAlias() with getContainerExtensionClass()

andrerom · 2017-11-16T17:17:40Z

composer.json

    "minimum-stability": "stable",
    "require": {
-        "ezsystems/ezpublish-kernel": "^6.7@dev || ^7.0@dev",
+        "ezsystems/ezpublish-kernel": "~6.7.7 || ^6.12.1 || ^7.0",


you'll need to keep dev for tests to continue to work

fixed in 358a682

Fixed again in b8a2348

please pull in changes before rebasing ;)

alongosz · 2017-11-17T08:29:06Z

docs/drivers.md

+services:
+    ezplatform.http_cache_myhttpcachebundle.purge_client.myhttpcache:
+        class: EzSystems\PlatformMyHttpCacheBundle\PurgeClient\MyHttpCachePurgeClient
+        arguments: ["@ezplatform.http_cache.cache_manager"]


Nitpick: Let's advocate in doc single quotes as well here. Sorry to be persistent and boring, just trying to clean the mess I made some time ago ;)

no reason to apologize. I guess I should use single quotes in the actuall services.yml to then... Will fix

alongosz · 2017-11-17T08:33:56Z

src/EzSystemsPlatformHttpCacheBundle.php

+        if ($this->extension) {
+            return $this->extension;
+        }
+    }


Why can't we just return new EzPlatformHttpCacheExtension()? See Symfony doc

ah.... man.... This we talked about yesterday
Ok, I'll change it again

Sorry, I think I missed the point of that discussion and didn't see this code yesterday. Doing too many things at the same time ;)

alongosz · 2017-11-17T08:35:14Z

src/DependencyInjection/Compiler/DriverPass.php

+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+class DriverPass implements CompilerPassInterface


Nitpick: Please add brief description on what this Compiler Pass does.

…tpCacheExtension class

vidarl commented Nov 2, 2017

View reviewed changes

vidarl mentioned this pull request Nov 2, 2017

EZP-28243: Purge type using service id does not work ezsystems/ezpublish-kernel#2136

Merged

andrerom reviewed Nov 5, 2017

View reviewed changes

Plopix reviewed Nov 5, 2017

View reviewed changes

vidarl force-pushed the plugin_support branch from f71ee94 to 7030246 Compare November 7, 2017 13:27

vidarl changed the title ~~Plugin support~~ WIP Plugin support Nov 8, 2017

vidarl changed the title ~~WIP Plugin support~~ [WIP] Plugin support Nov 8, 2017

vidarl force-pushed the plugin_support branch from 7030246 to 8e4595d Compare November 8, 2017 10:30

Revert "Allows http purge type to work (ezsystems#22)"

f53a80b

This reverts commit 6b1dafa.

vidarl force-pushed the plugin_support branch 3 times, most recently from a3e83d7 to 5702f5f Compare November 15, 2017 11:04

vidarl changed the title ~~[WIP] Plugin support~~ Plugin support Nov 15, 2017

andrerom reviewed Nov 15, 2017

View reviewed changes

andrerom approved these changes Nov 15, 2017

View reviewed changes

DominikaK reviewed Nov 15, 2017

View reviewed changes

vidarl force-pushed the plugin_support branch from 03f66f2 to aeb62eb Compare November 16, 2017 09:19

andrerom reviewed Nov 16, 2017

View reviewed changes

vidarl force-pushed the plugin_support branch from aeb62eb to 5e4295f Compare November 16, 2017 13:41

vidarl added 2 commits November 16, 2017 14:42

Added plugin support for purge clients

464a443

Renamed FOSPurgeClient.php to VarnishPurgeClient.php

ed61582

vidarl force-pushed the plugin_support branch from 5e4295f to fb34440 Compare November 16, 2017 14:02

andrerom reviewed Nov 16, 2017

View reviewed changes

andrerom requested a review from alongosz November 16, 2017 18:05

andrerom changed the title ~~Plugin support~~ EZEE-1780: Plugin support Nov 17, 2017

alongosz requested changes Nov 17, 2017

View reviewed changes

vidarl force-pushed the plugin_support branch from 358a682 to 206d19d Compare November 17, 2017 09:48

alongosz approved these changes Nov 17, 2017

View reviewed changes

vidarl added 6 commits November 17, 2017 11:39

Added plugin support for TagHandler

66bd765

Updated declaration of PurgeClientInterface

61691d1

Added doc on how to write http cache drivers

d6012e7

Changed DI alias to ez_platform_cache by renaming EzSystemsPlatformHt…

4cd4ed8

…tpCacheExtension class

Updated composer.json as we depend on ezpublish-kernel PR #2136

5dc3ae1

Changed to single quites on services.yml

14344d1

vidarl force-pushed the plugin_support branch from d7e00cf to 14344d1 Compare November 17, 2017 10:40

andrerom merged commit 750407d into ezsystems:master Nov 17, 2017

vidarl mentioned this pull request Nov 27, 2017

Fixed EZP-28243, BC break caused by EZEE-1780 #38

Closed

		@@ -14,7 +14,7 @@
		* Only support BAN requests on purpose, to be able to invalidate cache for a


		## FOS TagHandler

		The FOS Http cache bundle also have a TagHandler which is not used by eZ Platform except for one thing, the

Conversation

vidarl commented Nov 2, 2017 • edited by andrerom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vidarl Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plopix Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Plopix Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plopix Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plopix Nov 5, 2017 • edited by andrerom Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vidarl Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominikaK left a comment

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!

Choose a reason for hiding this comment

vidarl commented Nov 2, 2017 •

edited by andrerom

Loading

vidarl Nov 2, 2017 •

edited

Loading

andrerom Nov 5, 2017 •

edited

Loading

Plopix Nov 8, 2017 •

edited

Loading

andrerom Nov 5, 2017 •

edited

Loading

Plopix Nov 5, 2017 •

edited

Loading

andrerom Nov 6, 2017 •

edited

Loading

Plopix Nov 6, 2017 •

edited

Loading

Plopix Nov 5, 2017 •

edited by andrerom

Loading

vidarl Nov 15, 2017 •

edited

Loading

andrerom Nov 15, 2017 •

edited

Loading

andrerom left a comment •

edited

Loading