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

[DependencyInjection] deprecate access to private shared services.#19146

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

Merged

fabpot merged 1 commit intosymfony:masterfromhhamon:private-services-fix

Jun 29, 2016

Merged

[DependencyInjection] deprecate access to private shared services.#19146

fabpot merged 1 commit intosymfony:masterfromhhamon:private-services-fix

Jun 29, 2016

Conversation

Copy link

Contributor

hhamon commentedJun 22, 2016•
edited
Loading

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

carsonbot added Status: Needs Review DependencyInjection Deprecation labels

Jun 22, 2016

hhamon force-pushed theprivate-services-fix branch 4 times, most recently from02e1480 to9af32f8Compare

June 22, 2016 20:23

Copy link

ContributorAuthor

hhamon commentedJun 22, 2016

Looks like fabbot gives a false positive!

Copy link

Member

Choose a reason for hiding this comment

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

You also need to add a note in the component CHANGELOG file.

Copy link

Member

I would have implemented this feature in a different way by renaming internally private services so that their original name is not available from the container (this is what we are doing for anonymous services for instance).

Copy link

Member

xabbuh commentedJun 23, 2016

I think it's a good idea to switch to generated ids in 4.0. For now I like this approach as it allows users to fix their applications more smoothly.

javiereguiluz reviewed

Jun 23, 2016

View reviewed changes

src/Symfony/Component/DependencyInjection/Container.php Outdated

		$id =$lcId;
		}else {
		if (isset($this->privates[$id])) {
		@trigger_error(sprintf('Checking for the existence of the private service "%s" is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. An exception will be thrown instead in Symfony 4.0.',$id),E_USER_DEPRECATED);

Copy link

Member

javiereguiluzJun 23, 2016

Choose a reason for hiding this comment

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

Why throw an exception in this case? If you check a private service for existence, the method should returnfalse, right?

Copy link

Member

fabpotJun 23, 2016

Choose a reason for hiding this comment

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

Yes

Copy link

ContributorAuthor

hhamonJun 23, 2016

Choose a reason for hiding this comment

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

Yes it makes more sense!

hhamon changed the title~~[DependencyInjection] deprecate access to private shared services.~~[WIP] [DependencyInjection] deprecate access to private shared services.

Jun 23, 2016

Copy link

Member

fabpot commentedJun 23, 2016

@xabbuh Fair enough, let's keep the current approach for now.

Copy link

ContributorAuthor

hhamon commentedJun 23, 2016

I'll update the code later today to reflect your comments.

sstok reviewed

Jun 23, 2016

View reviewed changes

src/Symfony/Component/DependencyInjection/Container.php Outdated

		}

		if (isset($this->privates[$id])) {
		@trigger_error(sprintf('Resetting the private service "%s" from outside of the container is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. An exception will be thrown instead in Symfony 4.0.',$id),E_USER_DEPRECATED);

Copy link

Contributor

sstokJun 23, 2016

Choose a reason for hiding this comment

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

. and is part of new sentence part and should start with an uppercase 👍

Copy link

ContributorAuthor

hhamonJun 23, 2016

Choose a reason for hiding this comment

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

Where is that?

Copy link

Member

stof commentedJun 23, 2016

@fabpot generated ids are a good idea for 4.0 indeed, but this cannot be done for deprecations. If we want to switch to generated ids already, we would have to build another feature to to store a mapping between shared private service ids and their generated ids, to retrieve them this way and trigger a deprecation, which would again look like this approach.

hhamon force-pushed theprivate-services-fix branch 3 times, most recently from18ba656 to19f3c66Compare

June 23, 2016 19:41

Copy link

ContributorAuthor

hhamon commentedJun 23, 2016

I don't understand why the tests suite fails on PHP 5.5 and HHVM. The failing test in the ProxyManager Bridge passes on my machine with PHP 5.5.36...

fabpot reviewed

Jun 24, 2016

View reviewed changes

src/Symfony/Component/DependencyInjection/Container.php Outdated

		return;
		}
		if (isset($this->privates[$id])) {
		@trigger_error(sprintf('Requesting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. An exception will be thrown instead in Symfony 4.0.',$id),E_USER_DEPRECATED);

Copy link

Member

fabpotJun 24, 2016

Choose a reason for hiding this comment

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

I won't necessarily throw an exception as it depends on the$invalidBehavior value.

Copy link

ContributorAuthor

hhamonJun 24, 2016

Choose a reason for hiding this comment

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

yes that's right!

Copy link

Contributor

GuilhemN commentedJun 24, 2016

The tests are missing for theContainerBuilder :)

Copy link

ContributorAuthor

hhamon commentedJun 24, 2016

Thanks @Ener-Getick! I'm going to add some ;)

hhamon force-pushed theprivate-services-fix branch 2 times, most recently froma43a537 to13edce2Compare

June 27, 2016 11:03

hhamon changed the title~~[WIP] [DependencyInjection] deprecate access to private shared services.~~[DependencyInjection] deprecate access to private shared services.

Jun 27, 2016

Copy link

ContributorAuthor

hhamon commentedJun 27, 2016

I guess it's now ready for another round of reviews if tests pass.

hhamon force-pushed theprivate-services-fix branch from13edce2 to326cbebCompare

June 27, 2016 11:13

Copy link

Member

nicolas-grekas commentedJun 27, 2016

👍 (fabbot fails on a fixture, unrelated)

nicolas-grekas mentioned this pull request

Jun 27, 2016

Deprecate Container::set() for non-synthetic services#19192

Closed

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels

Jun 27, 2016

fabpot reviewed

Jun 28, 2016

View reviewed changes

UPGRADE-4.0.md Outdated

		no longer supported. Only public services can be set or unset.

		* Checking the existence of a private service with the`Container::has()`
		method is no longer supported.

Copy link

Member

fabpotJun 28, 2016

Choose a reason for hiding this comment

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

It's not that it is not supported anymore but more that it's going to return false.

Copy link

Member

fabpot commentedJun 28, 2016

Apart from my minor comment, 👍

[DependencyInjection] deprecate access to private shared services. Fi…

4ed70c4

…xes issuesymfony#19117.

hhamon force-pushed theprivate-services-fix branch from326cbeb to4ed70c4Compare

June 28, 2016 07:16

Copy link

ContributorAuthor

hhamon commentedJun 28, 2016

@fabpot fixed! CI is running ;)

Copy link

ContributorAuthor

hhamon commentedJun 28, 2016

Good to go!

Copy link

Member

fabpot commentedJun 29, 2016

Thank you@hhamon.

fabpot merged commit4ed70c4 intosymfony:master

Jun 29, 2016

fabpot added a commit that referenced this pull request

Jun 29, 2016

feature#19146[DependencyInjection] deprecate access to private shar…

ed6267d

…ed services. (hhamon)This PR was merged into the 3.2-dev branch.Discussion----------[DependencyInjection] deprecate access to private shared services.| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#19117| License       | MIT| Doc PR        | ~Commits-------4ed70c4 [DependencyInjection] deprecate access to private shared services. Fixes issue#19117.

linaori mentioned this pull request

Jul 12, 2016

Controller as a Service cookbook article missing failfast mentioningsymfony/symfony-docs#6749

Closed

ogizanagi mentioned this pull request

Aug 20, 2016

[DI] Deprecate Container::set for nulls, initialized and alias services#19668

Closed

ro0NL mentioned this pull request

Aug 20, 2016

[DI] Randomize private services#19683

Closed

bestform mentioned this pull request

Sep 2, 2016

Private services stay public when required by more than one service#12588

Closed

fabpot mentioned this pull request

Oct 27, 2016

Release v3.2.0-BETA1#20317

Merged

hhamon deleted the private-services-fix branch

February 22, 2017 13:13

ro0NL mentioned this pull request

May 20, 2017

[DI] Removed deprecated setting private/pre-defined services#22801

Merged

fabpot added a commit that referenced this pull request

Jul 17, 2017

feature#22801[DI] Removed deprecated setting private/pre-defined se…

286eb4d

…rvices (ro0NL)This PR was merged into the 4.0-dev branch.Discussion----------[DI] Removed deprecated setting private/pre-defined services| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | yes| Deprecations? | no| Tests pass?   | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->See#21533,#19708 and#19146@nicolas-grekas privates should be excluded from `getServiceIds` right? i also wondered.. did we forget to deprecate checking privates with `initialized()`?Commits-------9f96952 [DI] Removed deprecated setting private/pre-defined services

Labels

DependencyInjection Deprecation Status: Reviewed


		* Requesting a private shared service with the`get()` method of the`Container`
		class is no longer supported and will raise an exception.

Movatterモバイル変換

Uh oh!

[DependencyInjection] deprecate access to private shared services.#19146

[DependencyInjection] deprecate access to private shared services.#19146

Uh oh!

Conversation

hhamon commentedJun 22, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

hhamon commentedJun 22, 2016

Uh oh!

fabpot commentedJun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedJun 23, 2016

Uh oh!

xabbuh commentedJun 23, 2016

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!

fabpot commentedJun 23, 2016

Uh oh!

hhamon commentedJun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commentedJun 23, 2016

Uh oh!

hhamon commentedJun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemN commentedJun 24, 2016

Uh oh!

hhamon commentedJun 24, 2016

Uh oh!

hhamon commentedJun 27, 2016

Uh oh!

nicolas-grekas commentedJun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedJun 28, 2016

Uh oh!

hhamon commentedJun 28, 2016

Uh oh!

hhamon commentedJun 28, 2016

Uh oh!

fabpot commentedJun 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

hhamon commentedJun 22, 2016•
edited
Loading