Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesigned module architecture #6

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Redesigned module architecture #6

wants to merge 9 commits into from

Conversation

marcelo-lipienski
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

  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
Copy link
Contributor Author

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

The split is to ensure SRP.

$listener->attach($em);
$listener = $sm->get('CacheListener');

$em->attach($listener);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, you proxy to two methods you don't actually need to call. attach() meant for single listeners, not aggregated listeners. An aggregate is proxied to attachAggregate(). The aggregate attachment method is then proxied to attach() on the aggregate itself.

If you revert this change, it will save you two calls :)

@juriansluiman
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants