Skip to content

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

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
wants to merge 4 commits into from
Closed

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1088d62
[DependencyInjection] Deprecate ConstraintValidatorFactory::$validato…
GuilhemN Feb 23, 2017
7505eb8
[DependencyInjection] Use a service locator in AddConstraintValidator…
GuilhemN Feb 23, 2017
2928fbe
Update the CHANGELOG
GuilhemN Feb 24, 2017
7518b5c
Ignore abstract definitions
GuilhemN Mar 1, 2017
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
5 changes: 5 additions & 0 deletions UPGRADE-3.3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ FrameworkBundle
deprecated and will be removed in 4.0. Use the `Symfony\Component\PropertyInfo\DependencyInjection\PropertyInfoPass`
class instead.

* The `ConstraintValidatorFactory::$validators` and `$container` properties
have been deprecated and will be removed in 4.0.

* Extending `ConstraintValidatorFactory` is deprecated and won't be supported in 4.0.

HttpKernel
-----------

Expand Down
9 changes: 7 additions & 2 deletions UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ FrameworkBundle
removed. Use the `Symfony\Component\PropertyInfo\DependencyInjection\PropertyInfoPass`
class instead.

* The `ConstraintValidatorFactory::$validators` and `$container` properties
have been removed.

* Extending `ConstraintValidatorFactory` is not supported anymore.

HttpFoundation
---------------

Expand Down Expand Up @@ -243,7 +248,7 @@ HttpKernel

* The `Psr6CacheClearer::addPool()` method has been removed. Pass an array of pools indexed
by name to the constructor instead.

* The `LazyLoadingFragmentHandler::addRendererService()` method has been removed.

* The `X-Status-Code` header method of setting a custom status code in the
Expand Down Expand Up @@ -310,7 +315,7 @@ Translation
TwigBundle
----------

* The `ContainerAwareRuntimeLoader` class has been removed. Use the
* The `ContainerAwareRuntimeLoader` class has been removed. Use the
Twig `Twig_ContainerRuntimeLoader` class instead.

TwigBridge
Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ CHANGELOG
* Deprecated `FormPass`, use `Symfony\Component\Form\DependencyInjection\FormPass` instead
* Deprecated `SessionListener`
* Deprecated `TestSessionListener`
* Deprecated `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ConfigCachePass`.
* Deprecated `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ConfigCachePass`.
Use `Symfony\Component\Console\DependencyInjection\ConfigCachePass` instead.
* Deprecated `PropertyInfoPass`, use `Symfony\Component\PropertyInfo\DependencyInjection\PropertyInfoPass` instead
* Deprecated extending `ConstraintValidatorFactory`

3.2.0
-----
Expand All @@ -31,7 +32,7 @@ CHANGELOG
* Removed `symfony/asset` from the list of required dependencies in `composer.json`
* The `Resources/public/images/*` files have been removed.
* The `Resources/public/css/*.css` files have been removed (they are now inlined in TwigBundle).
* Added possibility to prioritize form type extensions with `'priority'` attribute on tags `form.type_extension`
* Added possibility to prioritize form type extensions with `'priority'` attribute on tags `form.type_extension`

3.1.0
-----
Expand Down
19 changes: 8 additions & 11 deletions ...mfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConstraintValidatorsPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@

namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Reference;

class AddConstraintValidatorsPass implements CompilerPassInterface
{
Expand All @@ -25,23 +26,19 @@ public function process(ContainerBuilder $container)

$validators = array();
foreach ($container->findTaggedServiceIds('validator.constraint_validator') as $id => $attributes) {
if (isset($attributes[0]['alias'])) {
$validators[$attributes[0]['alias']] = $id;
}

$definition = $container->getDefinition($id);

if (!$definition->isPublic()) {
throw new InvalidArgumentException(sprintf('The service "%s" must be public as it can be lazy-loaded.', $id));
if ($definition->isAbstract()) {
continue;
}

if ($definition->isAbstract()) {
throw new InvalidArgumentException(sprintf('The service "%s" must not be abstract as it can be lazy-loaded.', $id));
if (isset($attributes[0]['alias'])) {
$validators[$attributes[0]['alias']] = new Reference($id);
}

$validators[$definition->getClass()] = $id;
$validators[$definition->getClass()] = new Reference($id);
}

$container->getDefinition('validator.validator_factory')->replaceArgument(1, $validators);
$container->getDefinition('validator.validator_factory')->replaceArgument(0, new ServiceLocatorArgument($validators));
}
}
3 changes: 1 addition & 2 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@
</service>

<service id="validator.validator_factory" class="Symfony\Bundle\FrameworkBundle\Validator\ConstraintValidatorFactory" public="false">
<argument type="service" id="service_container" />
<argument type="collection" />
<argument type="service-locator" /> <!-- Constraint validators locator -->
</service>

<service id="validator.expression" class="Symfony\Component\Validator\Constraints\ExpressionValidator">
Expand Down
62 changes: 20 additions & 42 deletions ...le/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConstraintValidatorsPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,56 +13,34 @@

use PHPUnit\Framework\TestCase;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConstraintValidatorsPass;
use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;

class AddConstraintValidatorsPassTest extends TestCase
{
public function testThatConstraintValidatorServicesAreProcessed()
{
$services = array(
'my_constraint_validator_service1' => array(0 => array('alias' => 'my_constraint_validator_alias1')),
'my_constraint_validator_service2' => array(),
);

$validatorFactoryDefinition = $this->getMockBuilder('Symfony\Component\DependencyInjection\Definition')->getMock();
$container = $this->getMockBuilder('Symfony\Component\DependencyInjection\ContainerBuilder')->setMethods(array('findTaggedServiceIds', 'getDefinition', 'hasDefinition'))->getMock();

$validatorDefinition1 = $this->getMockBuilder('Symfony\Component\DependencyInjection\Definition')->setMethods(array('getClass'))->getMock();
$validatorDefinition2 = $this->getMockBuilder('Symfony\Component\DependencyInjection\Definition')->setMethods(array('getClass'))->getMock();

$validatorDefinition1->expects($this->atLeastOnce())
->method('getClass')
->willReturn('My\Fully\Qualified\Class\Named\Validator1');
$validatorDefinition2->expects($this->atLeastOnce())
->method('getClass')
->willReturn('My\Fully\Qualified\Class\Named\Validator2');

$container->expects($this->any())
->method('getDefinition')
->with($this->anything())
->will($this->returnValueMap(array(
array('my_constraint_validator_service1', $validatorDefinition1),
array('my_constraint_validator_service2', $validatorDefinition2),
array('validator.validator_factory', $validatorFactoryDefinition),
)));

$container->expects($this->atLeastOnce())
->method('findTaggedServiceIds')
->will($this->returnValue($services));
$container->expects($this->atLeastOnce())
->method('hasDefinition')
->with('validator.validator_factory')
->will($this->returnValue(true));

$validatorFactoryDefinition->expects($this->once())
->method('replaceArgument')
->with(1, array(
'My\Fully\Qualified\Class\Named\Validator1' => 'my_constraint_validator_service1',
'my_constraint_validator_alias1' => 'my_constraint_validator_service1',
'My\Fully\Qualified\Class\Named\Validator2' => 'my_constraint_validator_service2',
));
$container = new ContainerBuilder();
$validatorFactory = $container->register('validator.validator_factory')
->setArguments(array(new ServiceLocatorArgument()));
Copy link
Member

Choose a reason for hiding this comment

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

broken here, the constructor expects an argument. See #21821


$container->register('my_constraint_validator_service1', Validator1::class)
->addTag('validator.constraint_validator', array('alias' => 'my_constraint_validator_alias1'));
$container->register('my_constraint_validator_service2', Validator2::class)
->addTag('validator.constraint_validator');
$container->register('my_abstract_constraint_validator')
->setAbstract(true)
->addTag('validator.constraint_validator');

$addConstraintValidatorsPass = new AddConstraintValidatorsPass();
$addConstraintValidatorsPass->process($container);

$this->assertEquals(new ServiceLocatorArgument(array(
Validator1::class => new Reference('my_constraint_validator_service1'),
'my_constraint_validator_alias1' => new Reference('my_constraint_validator_service1'),
Validator2::class => new Reference('my_constraint_validator_service2'),
)), $validatorFactory->getArgument(0));
}

public function testThatCompilerPassIsIgnoredIfThereIsNoConstraintValidatorFactoryDefinition()
Expand Down
16 changes: 11 additions & 5 deletions src/Symfony/Bundle/FrameworkBundle/Validator/ConstraintValidatorFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

namespace Symfony\Bundle\FrameworkBundle\Validator;

use Symfony\Component\DependencyInjection\ContainerInterface;
use Psr\Container\ContainerInterface;
Copy link
Member

Choose a reason for hiding this comment

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

Given $container is protected I think that's a BC break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's acceptable, the methods defined are the same and it will only fail if someone checks its interface which is unlikely.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure. ContainerInterface contains more methods than the Psr one, calling $this->container->getParameter() would break. We had to write a BC layer for a similar case in #21451

Copy link
Member

@chalasr chalasr Feb 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Note also that adding deprecations for using experimental features has been rejected in #21625. I would wait for 4.0 on this one (that's why I did not proposed it), let's see what other think.

Copy link
Member

Choose a reason for hiding this comment

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

about deprecating in this PR, see #21625 (comment)
this needs to be resolved before acting to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've an idea of a BC layer (including deprecations but not about experimental features), I'll update the pr soon.

use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidatorFactoryInterface;
use Symfony\Component\Validator\ConstraintValidatorInterface;
Expand All @@ -37,6 +37,8 @@
* }
*
* @author Kris Wallsmith <kris@symfony.com>
*
* @final since version 3.3
*/
class ConstraintValidatorFactory implements ConstraintValidatorFactoryInterface
{
Expand Down Expand Up @@ -70,11 +72,15 @@ public function getInstance(Constraint $constraint)
$name = $constraint->validatedBy();

if (!isset($this->validators[$name])) {
if (!class_exists($name)) {
throw new ValidatorException(sprintf('Constraint validator "%s" does not exist or it is not enabled. Check the "validatedBy" method in your constraint class "%s".', $name, get_class($constraint)));
}
if ($this->container->has($name)) {
$this->validators[$name] = $this->container->get($name);
} else {
if (!class_exists($name)) {
throw new ValidatorException(sprintf('Constraint validator "%s" does not exist or it is not enabled. Check the "validatedBy" method in your constraint class "%s".', $name, get_class($constraint)));
}

$this->validators[$name] = new $name();
$this->validators[$name] = new $name();
}
} elseif (is_string($this->validators[$name])) {
Copy link
Member

Choose a reason for hiding this comment

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

should has() be called here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? In case what is returned by the container is a string ?

Copy link
Member

Choose a reason for hiding this comment

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

get() is called if this check succeeds. We're checking has() before get() in the first check above so I'm wondering if we need it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above it's needed because we want to know when to fallback to manual instantiating, here it just depends of the exception we want, with a has the code will fail a few lines later with an InvalidArgumentException, otherwise it will fail in the container.
This should not happen unless the user entered a wrong service, imo it's not worth supporting it.

Copy link
Member

Choose a reason for hiding this comment

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

👍

$this->validators[$name] = $this->container->get($this->validators[$name]);
}
Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
"symfony/serializer": "~3.3",
"symfony/translation": "~2.8|~3.0",
"symfony/templating": "~2.8|~3.0",
"symfony/validator": "~3.2",
"symfony/validator": "~3.3",
"symfony/yaml": "~3.2",
"symfony/property-info": "~3.3",
"doctrine/annotations": "~1.0",
Expand All @@ -65,7 +65,8 @@
"symfony/console": "<3.3",
"symfony/serializer": "<3.3",
"symfony/form": "<3.3",
"symfony/property-info": "<3.3"
"symfony/property-info": "<3.3",
"symfony/validator": "<3.3"
},
"suggest": {
"ext-apcu": "For best performance of the system caches",
Expand Down