Skip to content

Fixed EZP-28243, BC break caused by EZEE-1780 #38

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.

Sign up for GitHub

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

Closed
vidarl wants to merge 1 commit into from
Closed

Fixed EZP-28243, BC break caused by EZEE-1780 #38

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/DependencyInjection/Compiler/DriverPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
use eZ\Publish\Core\MVC\Symfony\Cache\PurgeClientInterface;

/**
* We support http cache drivers to be provided by 3rd party bundles.
Expand All @@ -19,7 +20,16 @@ public function process(ContainerBuilder $container)
$purgeType = $container->getParameter('ezpublish.http_cache.purge_type');
$configuredPurgeClientServiceId = static::getTaggedService($container, 'ezplatform.http_cache.purge_client');
if ($configuredPurgeClientServiceId === null) {
throw new \InvalidArgumentException("No driver found being able to handle purge_type '$purgeType'.");
// 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) {
Copy link
Member Author

@vidarl vidarl Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@andrerom andrerom Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

isn't it possible to also get just the definition and check class that way?

Copy link
Member Author

@vidarl vidarl Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@andrerom andrerom Nov 27, 2017

Choose a reason for hiding this comment

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

hmm, you might be right. I'm unsure what is the most correct here. maybe @bdunogier has input on that :)

Copy link
Member Author

@vidarl vidarl Nov 27, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@andrerom andrerom Nov 27, 2017

Choose a reason for hiding this comment

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

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)

Copy link
Member

@adamwojs adamwojs Nov 27, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@mnocon mnocon Nov 27, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@vidarl vidarl Nov 28, 2017

Choose a reason for hiding this comment

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

so, what do we do? Fix as suggested by Adam (thanx btw), or remove possibility to use service id?

Copy link
Contributor

@andrerom andrerom Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

imo doc, one clear way of doing things is better than many :)

throw new \InvalidArgumentException('Invalid ezpublish.http_cache.purge_type, it needs to implement PurgeClientInterface.');
}
$configuredPurgeClientServiceId = $purgeType;
} else {
throw new \InvalidArgumentException("No driver found being able to handle purge_type '$purgeType'.");
}
}
$container->setAlias('ezplatform.http_cache.purge_client', $configuredPurgeClientServiceId);

Expand Down