8000 [DependencyInjection] Use a service locator in AddConstraintValidatorsPass by GuilhemN · Pull Request #21730 · symfony/symfony · GitHub
[go: up one dir, main page]
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 1 commit
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
Prev Previous commit
Next Next commit
[DependencyInjection] Use a service locator in AddConstraintValidator… 
…sPass
  • Loading branch information
@GuilhemN
GuilhemN committed Mar 1, 2017
commit 7505eb83593260ad960baa9896aff2cb5ac3b695
2 changes: 2 additions & 0 deletions UPGRADE-3.3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ FrameworkBundle
* 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
3 changes: 3 additions & 0 deletions UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ FrameworkBundle
* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\FormPass` class has been
removed. Use the `Symfony\Component\Form\DependencyInjection\FormPass` class instead.

<<<<<<< HEAD
* The `Symfony\Bundle\FrameworkBundle\EventListener\SessionListener` class has been removed.
Use the `Symfony\Component\HttpKernel\EventListener\SessionListener` class instead.

Expand All @@ -205,6 +206,8 @@ FrameworkBundle
* The `ConstraintValidatorFactory::$validators` and `$container` properties
have been removed.

* Extending `ConstraintValidatorFactory` is not supported anymore.

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

Expand Down
18 changes: 5 additions & 13 deletions ...mfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConstraintValidatorsPass.php
View file
Open in desktop 8000
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 @@ -26,22 +27,13 @@ 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;
$validators[$attributes[0]['alias']] = new Reference($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()) {
throw new InvalidArgumentException(sprintf('The service "%s" must not be abstract as it can be lazy-loaded.', $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));
}
Copy link
Member
@nicolas-grekas nicolas-grekas Feb 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Abstract services should be ignored ie you should "continue". Note that this applies to many other cases in the code base: many passes throw on abstract services, but now that tags can be inherited in child definitions, we should just "continue" instead. Deserves another PR if you'd like to do it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like ContainerBuilder::findTaggedServiceIds() deserves a new argument ;)
I can't do it this week so I opened #21761 in case someone is interested.

}
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
12 changes: 7 additions & 5 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,6 +13,8 @@

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

class AddConstraintValidatorsPassTest extends TestCase
{
Expand Down Expand Up @@ -55,11 +57,11 @@ public function testThatConstraintValidatorServicesAreProcessed()

$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',
));
->with(0, new ServiceLocatorArgument(array(
'My\Fully\Qualified\Class\Named\Validator1' => new Reference('my_constraint_validator_service1'),
'my_constraint_validator_alias1' => new Reference('my_constraint_validator_service1'),
'My\Fully\Qualified\Class\Named\Validator2' => new Reference('my_constraint_validator_service2'),
)));

$addConstraintValidatorsPass = new AddConstraintValidatorsPass();
$addConstraintValidatorsPass->process($container);
Expand Down
71 changes: 11 additions & 60 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,17 +37,12 @@
* }
*
* @author Kris Wallsmith <kris@symfony.com>
*
* @final since version 3.3
*/
class ConstraintValidatorFactory implements ConstraintValidatorFactoryInterface
{
/**
* @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below
*/
protected $container;

/**
* @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below
*/
protected $validators;

/**
Expand Down Expand Up @@ -77,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 All @@ -92,52 +91,4 @@ public function getInstance(Constraint $constraint)

return $this->validators[$name];
}

/**
* @internal
*/
public function __get($name)
{
if ('validators' === $name || 'container' === $name) {
@trigger_error(sprintf('Using the "%s::$%s" property is deprecated since version 3.3 as it will be removed/private in 4.0.', __CLASS__, $name), E_USER_DEPRECATED);
}

return $this->$name;
}

/**
* @internal
*/
public function __set($name, $value)
{
if ('validators' === $name || 'container' === $name) {
@trigger_error(sprintf('Using the "%s::$%s" property is deprecated since version 3.3 as it will be removed/private in 4.0.', __CLASS__, $name), E_USER_DEPRECATED);
}

$this->$name = $value;
}

/**
* @internal
*/
public function __isset($name)
{
if ('validators' === $name || 'container' === $name) {
@trigger_error(sprintf('Using the "%s::$%s" property is deprecated since version 3.3 as it will be removed/private in 4.0.', __CLASS__, $name), E_USER_DEPRECATED);
}

return isset($this->$name);
}

/**
* @internal
*/
public function __unset($name)
{
if ('validators' === $name || 'container' === $name) {
@trigger_error(sprintf('Using the "%s::$%s" property is deprecated since version 3.3 as it will be removed/private in 4.0.', __CLASS__, $name), E_USER_DEPRECATED);
}

unset($this->$name);
}
}
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
0