[WIP/RFC] Refactored CacheInvalidator into descreet units

[dantleech]

• Renamed and repurposed CacheInvalidator => CacheManager
• CacheManager now delegates to handlers
• Added handlers for each use case

Usage:

$cache = new Varnish();
$pathHandler = new PathHandler($cache);
$tagsHandler = new TagsHandler($cache);

$cacheManager = new CacheManager(array(
    'path' => $pathHandler,
    'tags' => $pathHandler,
));

// invalidate by path
$cacheManager->invalidate('path', '/path/to');

// invalidate by path with headers
$cacheManager->invalidate('path', '/path/to', array(
    'headers' => array(
        'X-Foo' => 'bar'
    )
);

// invalidate tags
$cacheManager->invalidate('tags', array('foo', 'bar'));

// refresh a path
$cacheManager->refresh('path', '/path/to');

// flush -- will flush all the cache implementations used
// by the handlers (once)
$cacheManager->flush();

Each handler has its own cache proxy instance. This would allow different cache
implementations to be used simultaneously if required.

NOTE: Tests are currently missing.

[dantleech]

We keep track of handlers that need "flushing"

[stof]

This class must be kept for BC reasons (it can be implemented on top of the new APIs though)

[stof]

Please write tests for your new code. I think none of your new classes can actually be instantiated without error

[dantleech]

@stof no tests yet as noted in the description. this is just a sketch atm.

[ddeboer]

Besides BC (as mentioned by @stof) a reason to keep invalidatePath(), invalidateRoute(), invalidateTags() etc. is that those methods provide good IDE auto-completion. The same cannot be said of this more generic invalidate() method.

[dantleech]

I think that a clever IDE (like the one I don't use) is clever enough to complete based on the documented return (i.e. @return HandlerInterface). Of course you don't get the documentation for the concrete implementation but its a good compromise.

[dbu]

the problem is also with knowing what to call. $handlerName is just a variable, whereas invalidateTags or invalidateRoute is explicit.

[dantleech]

Well the exception would be useful here and would enumerate ALL the
possibilities instead of the ones which we happen to define an API for.

On 2015-01-04 17:31, David Buchmann wrote:

In src/CacheManager.php [1]:

• public function __construct($handlers)
• {
• $this->handlers = $handlers;
• }
  +
• /**
• * Invoke the invalidate on the named cache handler
• * @see CacheHandlerInterface::invaidate
• * @param string $handlerName
• * @param mixed $subject
• * @param array $options
• */
• public function invalidate($handlerName, $subject, array $options
  = array())

the problem is also with knowing what to call. $handlerName is just a
variable, whereas invalidateTags or invalidateRoute is explicit.

Reply to this email directly or view it on GitHub [2].

Links:

[1]

#153 (diff)
[2]
https://github.com/FriendsOfSymfony/FOSHttpCache/pull/153/files#r22440816

[dbu]

sorry dan, but i am really -1 on this one. i don't see any benefit from a user point of view.
$cacheManager->invalidate('tags', array('foo', 'bar')); is just less clear to me than $cacheManager->invalidateTags(array('foo', 'bar'));. hiding that there are several options might make things less confusing on first sight but then its harder to figure out what exists. and you still need to call something to tag the response, which was one of the initial problems with the CacheManager of the bundle.

the arguments you need to pass to invalidate() depend on the type of invalidation (route object, path, tag name, ...) so the client code can not ignore what it does or be reusable for different invalidation concepts.

having additional non-standard invalidation methods with this architecture ends us up in CacheManager being a registry where you ask for the right type of invalidator by "service" name (but we pass the parameters, instead of returning the handler so that its method could be called directly). that looks like an antipattern to IOC to me. if there are other methods, lets directly inject a handler for that other method with its custom method names to the class that wants to invalidate.

i think the discussion started with object invalidation. architectural, i see the point of not implementing the object handling in the CacheInvalidator - just like the TagManager, this should be an ObjectManager and the flexibility we where discussing in there makes sense. so either we could directly inject an ObjectManager or provide a proxy method invalidateObject on the CacheInvalidator - that proxy method would just be to keep it user friendly and gather all methods in one class (which i guess is a facade to a lot of the FOSHttpCache library, in design pattern speak).

[ddeboer]

As we discussed in FriendsOfSymfony/FOSHttpCacheBundle#184 (comment), we agreed to open a PR (this one) on the basis of which we determine whether we

• drop the CacheManager/CacheInvalidator facade completely
• or refactor the facade if we think the handlers add too much complexity or configuration steps for new users.

I have to agree with @dbu and I think we should go for the second alternative. I prefer the user-friendliness and explicitness of the facade.

[dantleech]

Thanks for the responses.

Concerning the method signatures of the (current) CacheManager: Are we certain that these will not change as we add more ProxyCache implementations (with more features). Always having an array of options allows you some future-proofing of the API in addition to allowing you to pass options to "fine tune" certain implementations.

I am not really happy with the CacheManager solution as I presented it, it is just offering a shortcut to the services. In reality I would want to inject the specific services I need directly as required and bypass the CacheManager altogether.

So how about:

• Keep the CacheManager as a facade to the built-in functionality of the Bundle.
• Split the monolithic CacheInvalidator class nto discreet classes (f.e. the handler classes in this PR but consider droping the method signature requirements).
• Inject the discreet handler classes into the CacheManager.

[dbu]

looking again at the CacheManager i see that my assumption that we have a TagManager is wrong. i would agree to move the logic of the invalidateTag method into a TagHandler here in the library (and deprecate setTagsHeader / getTagsHeader but forward to TagHandler for now). in the bundle, most of https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/master/EventListener/TagSubscriber.php could move to an extended TagHandler class that also supports tagging a response. as well as fixing FriendsOfSymfony/FOSHttpCacheBundle#182 to not all act on the subscriber, but the TagHander instead.

for the other invalidateX they seem so simple that an additional handler around the proxy client seems too much - just inject the proxy client there if you don't want to use the facade. does that make sense?

[dbu]

i would explicitly inject a TagHandler to the CacheInvalidator, similar to how we inject the ProxyClient. https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/src/CacheInvalidator.php#L282-L284 would instead check if $this->tagHandler is set or not.

[dantleech]

I'm just not a fan of mixing (patterns?). If we have a CacheInvalidator where the TagHandler (and ObjectHander) are injected and "shortcut" methods which are proxies to the ProxyClient then we have:

__construct(ProxyClientInterface $p, TagHandler $th, ObjectHandler $oh)

Where ProxyClient is also a dependency of both the TagHandler and ObjectHandler. Which just lacks symmetry.

[dbu]

i see the CacheInvalidator as a facade to the various "stuff" that is used to invalidate. those things are so simple that another wrapper is just needless overhead.

i think we all agree that we need an ObjectHandler. lets build that one and keep it stand alone. proxying those call through CacheInvalidator has not much value. you know whether you handle objects or not in your code, and we should not encourage mixing objects and other approaches in the same code. the ObjectHandler might want to depend on CacheInvalidator in turn, for doing tags or the like.

[dantleech]

I guess we close this now.