search

juriansluiman / SlmCache

Zend Framework 2 module to support full page caching based on route names
Other
12 stars 8 forks source link

Redesigned module architecture #6

Open marcelo-lipienski opened 10 years ago

marcelo-lipienski commented 10 years ago

I did it for learning purposes, I'm guessing it's now easier to be extended (maybe not ?!) Commits are somewhat messy, I'm trying to do better ^^

If you find time to take a look at this PR and maybe give me some feedback, I'd appreciate.

juriansluiman commented 10 years ago

Nice to see the additions :)

A real short scan:

  1. Don't add your .idea to a .gitignore, use your personal global git config for that
  2. It's better to have factories for the classes, nice addition
  3. Can you elaborate on the split between the listener and the service?
marcelo-lipienski commented 10 years ago
  1. Oh, ok. I'll fix that ^^
  2. Wee \o_
  3. I thought that registering listeners would fit outside caching logic, maybe, but I can't think of a valid argument to support it.
marcelo-lipienski commented 10 years ago

Ok, I did some research and now I have the answer for the third question.

The split is to ensure SRP.

juriansluiman commented 10 years ago

The split is to ensure SRP.

To be honest, you can go into fully SRP and split every single bit, but there's often no need for it. Separation of concerns is only needed to a certain extend. Take for instance the service class which does route matching, cache fetching and cache storage all in the same class. You could create a RouteMatcher, a RouteMatch class, a CacheAccess service layer, CacheCommand execution classes and so on. But really, this goes way too far.

I'd say just combine the cache/route logic into the listener. If there happens to be more logic required for e.g. response storage (say, you cache the headers from the response as well) or route matching (e.g. more parameters you can tune per route), then we could split those parts from the listener.

A second thing: the ServiceLocatorAware shouldn't be needed. You can grab the config in the factory and inject it into the listener. So the factory is responsible for injecting the right config. You can convert the options into an Options class, which if created within a separate factory. An example is the ModuleOptions from my Soflomo\Blog with its own factory.

The options class is the best way to do it. However, for such a small module, injecting the $config['slm_cache'] is "good enough" for me.