Fixed EZP-28243, BC break caused by EZEE-1780 #38
Conversation
…may be a service id
| // BC Check. purge_type may also be a service name implementing PurgeClientInterface | ||
| $purgeType = $container->getParameter('ezpublish.http_cache.purge_type'); | ||
| if ($container->has($purgeType)) { | ||
| if (!$container->get($purgeType) instanceof PurgeClientInterface) { |
There was a problem hiding this comment.
I here do a $container->get() inside a kernel-pass, same as ezsystems/ezpublish-kernel@5b45826#diff-469a42b65cf73f41f143b52df89d1e70R74
This might not be a good idea....If the service has dependencies to other services, that might or might not work
There was a problem hiding this comment.
isn't it possible to also get just the definition and check class that way?
There was a problem hiding this comment.
Then you get class name as string ('\MyNamespace\PurgType\AwsomePurge"). AFAIK, you then would need to use reflection in order to check if it implements the interface
Is that the way to go?
There was a problem hiding this comment.
hmm, you might be right. I'm unsure what is the most correct here. maybe @bdunogier has input on that :)
There was a problem hiding this comment.
My two cents would be do remove support for having purge_type=[service_id] entirely, as it has always been a broken feature, and no-one have requested it to be fixed ---> no-one has ever used it + ezplatform_http_cache now has plugin support for 3rd party purgers
There was a problem hiding this comment.
I'm fine with that. But then clarify further in the Driver.md doc and double check with @mnocon if there are other places / doc that will need clarification for it (aka I assume he got the impression to use service id from somewhere)
There was a problem hiding this comment.
We should be able to do this by using:
$definition = $container->getDefinition('ezpublish.http_cache.purge_type');
if (!is_subclass_of($definition->getClass(), PurgeClientInterface::class)) {
// ...
}Something similar is done here: https://github.com/symfony/framework-bundle/blob/3.4/DependencyInjection/Compiler/CacheCollectorPass.php#L45
There was a problem hiding this comment.
Actually I've used purge_type tag at first (using the drivers.md doc - it was clear enough for me), but then after Vidar's suggestion: https://jira.ez.no/browse/EZP-28243?focusedCommentId=219154 I've decided to try again with purge_type: service_id to see if this approach works.
I think it's only mentioned in the doc once, here: https://github.com/ezsystems/ezpublish-kernel/pull/2136/files#diff-78c39a773f9efe5f9c22eca3a9f246fbR330
There was a problem hiding this comment.
so, what do we do? Fix as suggested by Adam (thanx btw), or remove possibility to use service id?
There was a problem hiding this comment.
imo doc, one clear way of doing things is better than many :)
|
Given usage by service id was not working before 1.12.1 (not out yet), I don't think this necessarily is a BC break. But if this is ok with everyone then it's of course good to make it consistent. |
|
Solved in doc here for now: 92848da @vidarl maybe you and @bdunogier can improve the doc in kernel as part of the cleanup on injecting that config from bundle. |
4 participants
In kernel, purge_type may be a service id. This was not taken into account in #19
This PR fixes that issue, ref. https://jira.ez.no/browse/EZP-28243