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

add CacheManagerInterface.php that will allow CacheManager.php to be … #1351

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

Warxcell
Copy link
Contributor

…decorated.

Q A
Branch? 2.0
Bug fix? no
New feature? yes
BC breaks? no I think
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

@coveralls
Copy link

coveralls commented Jan 18, 2021

Coverage Status

Coverage increased (+0.04%) to 83.692% when pulling 217a86a on Warxcell:cache_manager_interface into 469b178 on liip:2.x.

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 8, 2021

can you remove all the formatting changes from the PR?
also can you briefly explain the specific use case you want to achieve with this change?
ie. for what use case do you want to decorate the CacheManger?

@Warxcell
Copy link
Contributor Author

Warxcell commented Feb 8, 2021

Don't remember - I think I decided to go with Adapter Pattern in my case, but I think it's useful anyway. :)

@Warxcell
Copy link
Contributor Author

Warxcell commented Feb 8, 2021

Basically I needed to add $runtimeConfig for particular path, globally - not just for one file. So to avoid Repeat-Your-Self I could decorate CacheManager and do

if(stripos($path, 'XXX') !== false)  {
    $runtimeConfig['YY'] = ['xxx' => 'xxx']
}
return $this->decorated->getBrowserPath($path, $filter, $runtimeConfig.... );`

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 8, 2021

ok .. as long as there is a use-case I am fine adding an interface :)

@fbourigault
Copy link
Contributor

fbourigault commented Feb 16, 2021

The CacheManager has two responsibilities.

The first one, for the isStored, store, remove and resolve methods, is to find the right resolver and call the same method this resolver.

The second one, for the getBrowserPath, getRuntimePath and generateUrl, is to get an URL or a path.

I see here a nice opportunity to split CacheManager.

The code is quite complex, but I would start with introducing some kind of ResolverManagerInterface to move the first responsibility out of CacheManager.

@dbu
Copy link
Member

dbu commented Oct 5, 2021

i like the suggestion of @fbourigault - do you want to split the interface in the way he suggested @Warxcell ?

@dbu dbu added the Level: Enhancement ✨ This item involves an enhancement to existing functionality. label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Level: Enhancement ✨ This item involves an enhancement to existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants