-
Notifications
You must be signed in to change notification settings - Fork 5
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
Cache expression #16
base: master
Are you sure you want to change the base?
Cache expression #16
Conversation
2 similar comments
@@ -14,7 +14,8 @@ | |||
"ocramius/proxy-manager": "2.*", | |||
"psr/log": "1.*", | |||
"psr/cache": "1.0.*", | |||
"doctrine/annotations": "1.3.*" | |||
"doctrine/annotations": "1.3.*", | |||
"symfony/expression-language": "3.*" | |||
}, |
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.
expression-language can be put as suggested?
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.
If you put it as suggested you can't use @CacheExpression
annotation I think.
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.
you can put it in both require-dev (so you can test it) and in suggests.
Who requires usage of expression should include the package explictly
} | ||
|
||
$this->expressionLanguage = new ExpressionLanguage(new FilesystemAdapter('expr_cache')); | ||
} |
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 we convert this into service based? This way it will be easier if someone wants to use anything else 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.
Your talkin' about the new FilesystemAdapter
?
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.
Yep, but also maybe classes that extend the base ExpressionLanguage...
public function calculateCachePrefix() : string | ||
{ | ||
return 'xyz'; | ||
} |
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 we also update the readme ?
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.
Yep I will.
Conflicts: src/ProxyManager/CacheableClassTrait.php tests/CacheWrapperTest.php tests/Helpers/CacheableClass.php
1 similar comment
4 similar comments
4 similar comments
|
||
$expressionLanguage = new \ReflectionClass($container->getDefinition($config['expression_language'])->getClass()); | ||
if ($expressionLanguage->getName() !== ExpressionLanguage::class) { | ||
throw new CacheException(sprintf('You must provide a valid Expression Language service')); |
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.
Too exact class match. You should be able to provide anything that extends expressionLanguage.
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.
Of course I will fix it.
'emag.cache.expression.language'=> (new Definition(ExpressionLanguage::class))->addArgument(new Reference('emag.cache.filesystem.adapter')), | ||
]); | ||
} elseif ($config['expression_language']) { | ||
$container->setAlias('emag.cache.expression.language', $config['expression_language']); |
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.
i would reverse the if. it's cheaper... :)
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.
Will do.
@@ -23,6 +25,8 @@ | |||
*/ | |||
protected $readerForCacheMethod; | |||
|
|||
protected $__expressionLanguage; | |||
|
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.
what's with the __ ?
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.
To avoid parameter collision.
Will take a look into it, but it requires a few changes to be included in the new version with the refactor done for v5.0 |
551b036
to
7f19c25
Compare
9bc4395
to
3ea90d7
Compare
No description provided.