FriendsOfSymfony / FOSHttpCacheBundle

Use the FOSHttpCache library in your Symfony projects
http://foshttpcachebundle.readthedocs.io
Other
431 stars 80 forks source link

Refactor User Context HttpCache handling into its own class #170

Closed dbu closed 9 years ago

dbu commented 9 years ago

We provide a HttpCache class extending the symfony kernel to handle user context requests.

We should move all business logic into a separate class and have minimal impact on the actual kernel class. People could still use our kernel, or they could copy the few lines just like you copy varnish configuration.

@lolautruche wdyt?

lolautruche commented 9 years ago

Not sure it worths it honestly. Impact is already minimal (see handle()). And it will be almost impossible for future improvement such as tag based invalidation.

dbu commented 9 years ago

its still > 100 lines of code, for something that could be like 5 lines in the kernel. i think i will try a PR.

why do you think the tag based invalidation can not be encapsulated? in the end, we should have a class that is called from a few of the kernel methods to do its stuff. banning on headers needs a meta index, but i think even that could be completely encapsulated in that separate class (its only a meta-index to find cached content by header, right?)

lolautruche commented 9 years ago

why do you think the tag based invalidation can not be encapsulated?

Actually it will be part of the extended store. A few lines will have to be added to the HttpCache class to use the Store and deal with invalidation.

ddeboer commented 9 years ago

@dbu Please create that PR: I’d prefer composition over (HttpKernel) inheritance.

dbu commented 9 years ago

i created #177 now - indeed the kernel part becomes a lot more lean. but we end up with another layer of event handling, it looks like :-/

stof commented 9 years ago

Would it make sense to extract this code outside the bundle, to make it reusable for other people wanting to use the HttpCache (Silex, StackPHP, ...) ? The bundle could then depend on this library as well.

stof commented 9 years ago

Extracting it out of the bundle would even make more sense once we implement #168 and #167 as the number of extra HttpCache features will become bigger.

ddeboer commented 9 years ago

Though I generally agree with moving code to the library, I’m not sure about HttpCache. After all, the base class is from the FrameworkBundle, which seems a weird (optional) dependency for the lib. In our bundle, it makes more sense to assume the FrameworkBundle is available.

dbu commented 9 years ago

i first thought: if somebody wants this in silex, he should just require the bundle and live with a couple unused classes. but actually its not that easy, as this will pull in the whole framework bundle and with it most symfony components...

if we move this to the library, the library starts depending on the http kernel just for one feature many people will not use. we could of course declare that as an "optional dependency" but afaik this is frowned upon as well (though in this case it would be pretty obvious).

note that at this moment, the old HttpCache class we have here extends the framework bundle HttpCache. so we basically would have to copy the event stuff and bootstrapping into it and probably stop deprecating it, as it makes sense for a symfony full stack project to use the HttpCache from FrameworkBundle i guess (assuming that that class is not pointless...)

if we want to propose this in the library, i see 2 options:

  1. we make the http cache an optional dependency and provide two HttpCache, one in the lib based on kernel component, one here in the bundle based on frameworkbundle
  2. we only have the plugins in the library and just tell people to copy the HttpCache class from the bundle into their project and adjust the BaseHttpCache to the one from the kernel component. this would still create an optional dependency, but this time its very explicit that you need the kernel if you want your own kernel...

(obviously the library would start to require-dev the symfony kernel to test)

ddeboer commented 9 years ago

Yes, optional dependencies are bad (and an oxymoron) but frequently the only way to get rid of them is to create a separate package, e.g., http-cache-plugins or something similar that depends on http-cache. But adding packages would make most sense if our lib and bundle were in their own GitHub organisation. They being in FOS as they are now, I can accept the dependency.

So I'm for having the plugins in the library only, where they can be reused outside a full-stack Symfony project. I think that's your option 1.