[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

GuilhemN · 2017-02-23T11:42:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Use a service locator to load constraint validators: it allows them to be private.

chalasr · 2017-02-23T11:45:46Z

src/Symfony/Bundle/FrameworkBundle/Validator/ConstraintValidatorFactory.php

@@ -11,7 +11,7 @@

 namespace Symfony\Bundle\FrameworkBundle\Validator;

-use Symfony\Component\DependencyInjection\ContainerInterface;
+use Psr\Container\ContainerInterface;


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

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

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

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.

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

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

GuilhemN · 2017-02-23T19:20:47Z

Considering your comment, I think it's better to delay this change to 4.0 and to just deprecate ConstraintValidatorFactory::$container and ConstraintValidatorFactory::$validators for now, it will be much easier to do this way.

nicolas-grekas · 2017-02-23T19:37:37Z

Is there any BC break really?
I mean, code that exists won't suddenly get a PSR11-only object injected, will they?

GuilhemN · 2017-02-23T19:41:28Z

If someone uses $container he would receive a psr-11 only container as I updated the first argument to a service locator.
I think we can change the type hint now but we can't update the service config to a service locator before 4.0.

nicolas-grekas · 2017-02-23T19:43:08Z

If someone uses $container he would receive a psr-11 only container as I updated the first argument to a service locator

BC breaks are about existing code - ie already written ones - that may break because of a change here. How can this change break existing code?

chalasr · 2017-02-23T20:01:24Z

Anyone extending this code and wiring it on its own doesn't have to check that what it receives is a symfony container until now since we guarantee it, after this change such code would work by luck if it relies on any method that is not part of the PSR-11 replacement. It would truly break only when decorating the service of this factory, maybe we don't care.

GuilhemN · 2017-02-23T20:23:35Z

@chalasr as discussed with @nicolas-grekas, there is no bc break as it won't break existing code: the only time the container will no longer be an instance of the symfony container when upgrading will be when using the validator.validator_factory service but it doesn't matter as we know it's not extended (its class is fixed).
So maybe a class extending ConstraintValidatorFactory won't support the psr-11 container but that's not truly a bc break as it won't break existing code, the container will always be a symfony container, only new code could be broken but that's not part of bc.

chalasr · 2017-02-23T20:26:35Z

Pleased to see one more container injection removed then.

chalasr · 2017-02-24T09:25:10Z

src/Symfony/Bundle/FrameworkBundle/Validator/ConstraintValidatorFactory.php


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


should has() be called here too?

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

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

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.

chalasr · 2017-02-24T09:45:59Z

The Validator's CHANGELOG should mention the deprecation

GuilhemN · 2017-02-25T09:37:12Z

Changelog updated ☺

nicolas-grekas · 2017-02-25T10:45:23Z

UPGRADE-3.3.md

@@ -71,6 +71,9 @@ FrameworkBundle
   deprecated and will be removed in 4.0. Use the `Symfony\Component\Form\DependencyInjection\FormPass`
   class instead.

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


could be on one line

nicolas-grekas · 2017-02-25T10:47:58Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConstraintValidatorsPass.php

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


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 :)

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

…ument type (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Remove experimental status from service-locator argument type | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21625 (comment), #21625 (comment), #21710 | License | MIT The `service-locator` argument type is not controversial to me. We know its scope, nothing really surprising, just a map of services to be lazily loaded like `iterator` is (which is not experimental) but keyed. About its api, it's just PSR-11 restricted to objects, nothing that can't be changed safely in the future. As stated in #21625 (comment), it proven its usefulness already. I think what we were looking for by flagging it experimental is just to see it in action, we've 3 opened PRs for that (#21625, #21690, #21730). This allows introducing deprecations for making use of the feature in the core, thus unlocks #21625 and #21690. Commits ------- 46dc47a [DI] Remove experimental status from service-locator argument type

nicolas-grekas · 2017-02-28T13:00:43Z

rebase needed

…rs and $container

…sPass

GuilhemN · 2017-03-01T16:05:43Z

@nicolas-grekas done.

Also, abstract definitions are now ignored and I improved the test readability by removing mocks.

nicolas-grekas

👍

fabpot · 2017-03-01T16:17:35Z

Thank you @GuilhemN.

chalasr · 2017-03-01T17:24:30Z

...undle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConstraintValidatorsPassTest.php

-            ));
+        $container = new ContainerBuilder();
+        $validatorFactory = $container->register('validator.validator_factory')
+            ->setArguments(array(new ServiceLocatorArgument()));


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

carsonbot added Status: Needs Review DependencyInjection labels Feb 23, 2017

chalasr reviewed Feb 23, 2017

View reviewed changes

GuilhemN changed the title ~~[DependencyInjection] Use a service locator in AddConstraintValidatorsPass~~ [DependencyInjection] Deprecate ConstraintValidatorFactory::$validators and $container Feb 23, 2017

GuilhemN force-pushed the SERVICELOCATOR branch from e2f0107 to 9ccdeac Compare February 23, 2017 19:25

GuilhemN changed the title ~~[DependencyInjection] Deprecate ConstraintValidatorFactory::$validators and $container~~ [DependencyInjection] Use a service locator in AddConstraintValidatorsPass Feb 23, 2017

chalasr approved these changes Feb 24, 2017

View reviewed changes

nicolas-grekas reviewed Feb 25, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Feb 25, 2017

GuilhemN mentioned this pull request Feb 25, 2017

[DependencyInjection] Ignore tags of abstract services #21761

Closed

chalasr mentioned this pull request Feb 26, 2017

[DI] Remove experimental status from service-locator argument type #21766

Merged

GuilhemN added 4 commits March 1, 2017 16:40

[DependencyInjection] Deprecate ConstraintValidatorFactory::$validato…

1088d62

…rs and $container

[DependencyInjection] Use a service locator in AddConstraintValidator…

7505eb8

…sPass

Update the CHANGELOG

2928fbe

Ignore abstract definitions

7518b5c

GuilhemN force-pushed the SERVICELOCATOR branch from 439b4b1 to 7518b5c Compare March 1, 2017 16:02

nicolas-grekas approved these changes Mar 1, 2017

View reviewed changes

fabpot closed this in 69374da Mar 1, 2017

GuilhemN deleted the SERVICELOCATOR branch March 1, 2017 16:28

chalasr reviewed Mar 1, 2017

View reviewed changes

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

ogizanagi mentioned this pull request May 20, 2017

[FrameworkBundle] Remove deprecated code #22800

Merged

chalasr mentioned this pull request May 21, 2017

[FrameworkBundle][Validator] Deprecate passing validator instances/aliases over using the service locator #22808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

GuilhemN commented Feb 23, 2017 •

edited

Loading

chalasr Feb 23, 2017

GuilhemN Feb 23, 2017

chalasr Feb 23, 2017

chalasr Feb 23, 2017 •

edited

Loading

nicolas-grekas Feb 23, 2017

GuilhemN Feb 23, 2017

GuilhemN commented Feb 23, 2017

nicolas-grekas commented Feb 23, 2017 •

edited

Loading

GuilhemN commented Feb 23, 2017

nicolas-grekas commented Feb 23, 2017

chalasr commented Feb 23, 2017

GuilhemN commented Feb 23, 2017

chalasr commented Feb 23, 2017

chalasr Feb 24, 2017

GuilhemN Feb 24, 2017

chalasr Feb 24, 2017

GuilhemN Feb 24, 2017

chalasr Feb 24, 2017

chalasr commented Feb 24, 2017

GuilhemN commented Feb 25, 2017

nicolas-grekas Feb 25, 2017

GuilhemN Feb 25, 2017

nicolas-grekas Feb 25, 2017 •

edited

Loading

GuilhemN Feb 25, 2017

nicolas-grekas commented Feb 28, 2017

GuilhemN commented Mar 1, 2017

nicolas-grekas left a comment

fabpot commented Mar 1, 2017

chalasr Mar 1, 2017

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

Conversation

GuilhemN commented Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN commented Feb 23, 2017

nicolas-grekas commented Feb 23, 2017 • edited Loading

GuilhemN commented Feb 23, 2017

nicolas-grekas commented Feb 23, 2017

chalasr commented Feb 23, 2017

GuilhemN commented Feb 23, 2017

chalasr commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Feb 24, 2017

GuilhemN commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Feb 28, 2017

GuilhemN commented Mar 1, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Mar 1, 2017

Choose a reason for hiding this comment

GuilhemN commented Feb 23, 2017 •

edited

Loading

chalasr Feb 23, 2017 •

edited

Loading

nicolas-grekas commented Feb 23, 2017 •

edited

Loading

nicolas-grekas Feb 25, 2017 •

edited

Loading