NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

GuilhemN wants to merge4 commits intosymfony:masterfromGuilhemN:SERVICELOCATOR

Closed

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass#21730

GuilhemN wants to merge4 commits intosymfony:masterfromGuilhemN:SERVICELOCATOR

Conversation

Copy link

Contributor

GuilhemN commentedFeb 23, 2017•
edited
Loading

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.

carsonbot added Status: Needs Review DependencyInjection labels

Feb 23, 2017

chalasr reviewed

Feb 23, 2017

View reviewed changes

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

		namespaceSymfony\Bundle\FrameworkBundle\Validator;

		useSymfony\Component\DependencyInjection\ContainerInterface;
		usePsr\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

ContributorAuthor

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

chalasrFeb 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

ContributorAuthor

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.

Copy link

ContributorAuthor

GuilhemN commentedFeb 23, 2017

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

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

Feb 23, 2017

GuilhemN force-pushed theSERVICELOCATOR branch frome2f0107 to9ccdeacCompare

February 23, 2017 19:25

Copy link

Member

nicolas-grekas commentedFeb 23, 2017•
edited
Loading

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

Copy link

ContributorAuthor

GuilhemN commentedFeb 23, 2017

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.

Copy link

Member

nicolas-grekas commentedFeb 23, 2017

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?

Copy link

Member

chalasr commentedFeb 23, 2017

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.

Copy link

ContributorAuthor

GuilhemN commentedFeb 23, 2017

@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 thevalidator.validator_factory service but it doesn't matter as we know it's not extended (its class is fixed).
So maybe a class extendingConstraintValidatorFactory 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.

Copy link

Member

chalasr commentedFeb 23, 2017

Pleased to see one more container injection removed then.

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

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

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

Copy link

Member

chalasrFeb 24, 2017

Choose a reason for hiding this comment

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

shouldhas() be called here too?

Copy link

ContributorAuthor

GuilhemNFeb 24, 2017

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

chalasrFeb 24, 2017

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 checkinghas() beforeget() in the first check above so I'm wondering if we need it here

Copy link

ContributorAuthor

GuilhemNFeb 24, 2017

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 ahas 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

chalasrFeb 24, 2017

Choose a reason for hiding this comment

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

👍

Copy link

Member

chalasr commentedFeb 24, 2017

The Validator's CHANGELOG should mention the deprecation

Copy link

ContributorAuthor

GuilhemN commentedFeb 25, 2017

Changelog updated ☺

nicolas-grekas reviewed

Feb 25, 2017

View reviewed changes

UPGRADE-3.3.md Outdated

		class instead.

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

Copy link

Member

nicolas-grekasFeb 25, 2017

Choose a reason for hiding this comment

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

could be on one line

Copy link

ContributorAuthor

GuilhemNFeb 25, 2017

Choose a reason for hiding this comment

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

updated

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

		thrownewInvalidArgumentException(sprintf('The service "%s" must be public as it can be lazy-loaded.',$id));
		}

		if ($definition->isAbstract()) {

Copy link

Member

nicolas-grekasFeb 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

ContributorAuthor

GuilhemNFeb 25, 2017

Choose a reason for hiding this comment

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

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

nicolas-grekas added this to the3.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

fabpot added a commit that referenced this pull request

Feb 26, 2017

minor#21766[DI] Remove experimental status from service-locator arg…

fb65bd6

…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       | MITThe `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

Copy link

Member

nicolas-grekas commentedFeb 28, 2017

rebase needed

GuilhemN added4 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 theSERVICELOCATOR branch from439b4b1 to7518b5cCompare

March 1, 2017 16:02

Copy link

ContributorAuthor

GuilhemN commentedMar 1, 2017

@nicolas-grekas done.

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

nicolas-grekas approved these changes

Mar 1, 2017

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

Copy link

Member

fabpot commentedMar 1, 2017

Thank you@GuilhemN.

fabpot closed this in69374da

Mar 1, 2017

GuilhemN deleted the SERVICELOCATOR branch

March 1, 2017 16:28

chalasr reviewed

Mar 1, 2017

View reviewed changes

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

		));
		$container =newContainerBuilder();
		$validatorFactory =$container->register('validator.validator_factory')
		->setArguments(array(newServiceLocatorArgument()));

Copy link

Member

chalasrMar 1, 2017

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

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

Labels

DependencyInjection Status: Needs Review

Movatterモバイル変換

Uh oh!

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass#21730

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass#21730

Uh oh!

Conversation

GuilhemN commentedFeb 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasrFeb 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemN commentedFeb 23, 2017

Uh oh!

nicolas-grekas commentedFeb 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

GuilhemN commentedFeb 23, 2017

Uh oh!

nicolas-grekas commentedFeb 23, 2017

Uh oh!

chalasr commentedFeb 23, 2017

Uh oh!

GuilhemN commentedFeb 23, 2017

Uh oh!

chalasr commentedFeb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commentedFeb 24, 2017

Uh oh!

GuilhemN commentedFeb 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasFeb 25, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedFeb 28, 2017

Uh oh!

GuilhemN commentedMar 1, 2017

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commentedMar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

GuilhemN commentedFeb 23, 2017•
edited
Loading

chalasrFeb 23, 2017•
edited
Loading

nicolas-grekas commentedFeb 23, 2017•
edited
Loading

nicolas-grekasFeb 25, 2017•
edited
Loading