-
Notifications
You must be signed in to change notification settings - Fork 82
Refactor asset cache Manager + Zend Cache Adaptor #128
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
Conversation
|
@Ocramius - Have you had time to look this one over? Just wondering if I need to make any modifications while it's on my radar. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backwards compatibility compatibility? :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be. That was based on the conversation in the last PR, which sent me down this road. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I mean, you wrote "BC compatibility" but the "C" in "BC" stands for compatibility. So you wrote Backwards compatibility compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I just added it for comical relief. Pretty funny isn't it? :p I'll fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:p
config/module.config.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment on why this is still here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "I'm too lazy to find all the references to this" work? :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope :P
…ssetManager test to use a mock cache manager instead of the actual class, tests for the Cache Manager now have their own test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, what about BC? If anyone uses this they're boned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think they'll use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the alias back for that. Just in case. Also made a comment for @Ocramius. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesomesauce. I just really like BC. It's my favorite. Next to peanut butter jelly sandwiches, but that's logic.
|
@Ocramius - Okay, refactored so only the needed caches are being instantiated. See if this is more to your liking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, These were removed because they test the Asset Manager Cache and not the Asset Manager itself. The Asset Manager Cache is now being tested on it's own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docblock
|
@wshafer looks much better to me! |
Removed unneeded doc blocks Moved config to constructor + factory and updated unit tests for the asset cache manager. Changed order for asset cache provider discovery and return from method as soon as resource is found.
|
There ya go @Ocramius and @RWOverdijk . Refactored based on our previous conversations. |
|
Looking good to me. |
|
Mhm, agreed. You did one hell of a job. I think, to be on the safe side, I'm tagging this |
|
@wshafer could you add wiki entries for the new zend cache provider? |
|
@Ocramius - Of course. As long as someone proofs it. English writing is not one of my strengths (as you've probably already guessed). ;) |
|
@wshafer don't worry that much about it as long as there is understandable documentation. |
|
And I'll go through it. :) |
|
@RWOverdijk - Ok see if the new edits make sense. Thanks. |
|
@RWOverdijk & @Ocramius - I was just re-reading my wiki edits and one thing hit me. It is really complicated to configure the ZendCacheAdaptor. Would it make more sense for it to follow the current caches and add it to the class mapper? This would make it simple to use with config just like the others. Here's what I'm thinking. file: module.config.php return array(
'asset_manager' => array(
'caching' => array(
'default' => array(
'cache' => 'AssetManager\\Cache\\ZendCacheAdapter'
'options' => array(
'service' => 'myZendService'
),
),
),
),
);Thoughts? |
|
@wshafer well, that is pretty much the orientation that I took with anything related to including third-party services. |
|
I think that's @Ocramius his way of saying "good idea". Not sure. :p |
|
@Ocramius Do you want it added to the class mapper or can I merge? |
|
It can be done as a cleanup afterwards. |
Refactor asset cache Manager + Zend Cache Adaptor
Based on Ocramius review of PR #127, I have taken a stab at refactoring the Asset Cache Manger to pull services from the SM where possible.
The provider factory now accepts 4 ways to configure the cache. Callback, A Zend Service, Class name, and a BC mapper for things like "APC"
Possible way's to configure the caches are as follows:
Also attached is a Zend Cache Adaptor for Assetic. In order for this to work the adapter needs the cache injected. As such I have only set this up to be used via the service manager.
To configure this adapter here is one possible solution:
File: MyApplication/Module.php
And my config/autoload/local.php config would look like: