search

asm89 / twig-cache-extension

The missing cache extension for Twig. Caching template fragments with Twig.
MIT License
388 stars 27 forks source link

Support cache invalidation #14

Closed teohhanhui closed 10 years ago

asm89 commented 10 years ago

@teohhanhui Why does the cache invalidation need all these events?

teohhanhui commented 10 years ago

I store the cache key during the save event, so that I can delete it when appropriate. On May 31, 2014 9:40 PM, "Alexander" notifications@github.com wrote:

@teohhanhui https://github.com/teohhanhui Why does the cache invalidation need all these events?

— Reply to this email directly or view it on GitHub https://github.com/asm89/twig-cache-extension/pull/14#issuecomment-44748582 .

asm89 commented 10 years ago

@teohhanhui It would be easy for you to wrap the cache driver instead of adding all this event dispatching to the extension?

teohhanhui commented 10 years ago

Should the cache driver be involved in invalidation? Shouldn't there be a separation of concerns (i.e. the cache driver is only supposed to be used to store and retrieve cache blocks)? On May 31, 2014 9:53 PM, "Alexander" notifications@github.com wrote:

@teohhanhui https://github.com/teohhanhui It would be easy for you to wrap the cache driver instead of adding all this event dispatching to the extension?

— Reply to this email directly or view it on GitHub https://github.com/asm89/twig-cache-extension/pull/14#issuecomment-44748878 .

asm89 commented 10 years ago

With this PR, the strategy implementations are suddenly responsible for throwing events when a block is fetched or saved. In my opinion it would make more sense to push that down to the cache driver (or a wrapper object that throws events!), instead of putting the event logic in every strategy implementation.

stloyd commented 10 years ago

For me this PR mixes two features when each other has nothing two do with it. While I can understand point for invalidation, I totally don't get reasons for events (and if any implementation would land, it should be done like @asm89 said, additional cache provider that dispatches events if needed).

asm89 commented 10 years ago

See the other PR. Closing this one.