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

[ProxyManagerBridge] Fixed support of private services#27593

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

nicolas-grekas merged 1 commit intosymfony:3.4fromomnilight:di_fix_for_proxy_dumper

Jun 15, 2018

Merged

[ProxyManagerBridge] Fixed support of private services#27593

nicolas-grekas merged 1 commit intosymfony:3.4fromomnilight:di_fix_for_proxy_dumper

Jun 15, 2018

Conversation

Copy link

omnilight commentedJun 13, 2018

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

Fixed lazy loading of private services, that was broken since Symfony 4.0 release because of renaming
addObjectResource
https://github.com/symfony/symfony/blame/fa022f05be75aacc8afcdc11b63333685c719e13/src/Symfony/Component/DependencyInjection/CHANGELOG.md#L114

carsonbot added Status: Needs Review DependencyInjection Bug labels

Jun 13, 2018

omnilight force-pushed thedi_fix_for_proxy_dumper branch from0931007 toc016761Compare

June 13, 2018 14:53

rodnaph reviewed

Jun 13, 2018

View reviewed changes

src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/PhpDumper/ProxyDumperTest.php

		);
		}

		publicfunctiongetPrivatePublicDefinitions()

Copy link

Contributor

Choose a reason for hiding this comment

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

make static?

Copy link

Contributor

Choose a reason for hiding this comment

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

what for? it's phpunit dataprovider

Copy link

Contributor

Choose a reason for hiding this comment

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

avoids needing to create instance of test case to invoke

Copy link

Contributor

Choose a reason for hiding this comment

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

https://phpunit.de/manual/6.5/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers

Copy link

Contributor

Choose a reason for hiding this comment

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

https://github.com/sebastianbergmann/phpunit/blob/master/src/Util/Test.php#L840

using $this inside the data provider can be confusing if it's not static too, use of Prophecy for example isn't managed etc... it was just a minor suggestion.

Copy link

Author

omnilightJun 13, 2018•
edited
Loading

Choose a reason for hiding this comment

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

But nowhere in the Symfony tests data proiders are made static. Why must this one be?

carsonbot removed the Status: Needs Review label

Jun 13, 2018

jakzal suggested changes

Jun 13, 2018

View reviewed changes

src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/PhpDumper/ProxyDumperTest.php Outdated


		$code =$this->dumper->getProxyFactoryCode($definition,'foo','$this->getFoo2Service(false)');

		$this->assertStringMatchesFormat(

Copy link

Contributor

Choose a reason for hiding this comment

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

Should be one line to be consistent with the code base.

Copy link

Author

Choose a reason for hiding this comment

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

Fixed

src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/PhpDumper/ProxyDumperTest.php Outdated

		/**
		* @dataProvider getPrivatePublicDefinitions
		*/
		publicfunctiontestCorrectPublicPrivateInstantiation(Definition$definition,string$access)

Copy link

Contributor

Choose a reason for hiding this comment

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

I hate to be picky, but what doesn it mean "correct" in this context?

Copy link

Author

Choose a reason for hiding this comment

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

Changed name

carsonbot added the Status: Needs Work label

Jun 13, 2018

omnilight force-pushed thedi_fix_for_proxy_dumper branch fromc016761 toa4ba66fCompare

June 13, 2018 15:53

Copy link

Member

stof commentedJun 13, 2018

The issue is not the method name. It is the!. We want to useprivates only on 4.0 where the method is removed

Copy link

Member

stof commentedJun 13, 2018

And this needs to be fixed in 3.4, to account for ProxyManagerBridge and DependencyInjection being installed at different versions.

Copy link

Member

stof commentedJun 13, 2018

@nicolas-grekas can you confirm my diagnosis ?

Copy link

Author

omnilight commentedJun 13, 2018

Ok, so probably it is better just to remove checking for method existence and look only on service public or not?

Copy link

Author

omnilight commentedJun 13, 2018

And maybe check for the version...

Copy link

Member

stof commentedJun 14, 2018

Well, thismethod_exists is precisely the way we detect whether we are using DI 3.4 or 4.0. We don't have a version constant in the DI component.

Copy link

Author

omnilight commentedJun 14, 2018

Ok,@stof than we probably can check for both methods

Copy link

Member

stof commentedJun 14, 2018

Why checking for both methods ? We only need to check for a methodremoved in 4.0.

nicolas-grekas added this to the3.4 milestone

Jun 14, 2018

nicolas-grekas changed the base branch frommaster to3.4

June 14, 2018 10:03

nicolas-grekas force-pushed thedi_fix_for_proxy_dumper branch froma4ba66f to90dd156Compare

June 14, 2018 10:03

nicolas-grekas changed the title~~[DI] Fixed support of private services since Symfony 4.0~~[ProxyManagerBridge] Fixed support of private services

Jun 14, 2018

[ProxyManagerBridge] Fixed support of private services

198bee0

nicolas-grekas force-pushed thedi_fix_for_proxy_dumper branch from90dd156 to198bee0Compare

June 14, 2018 10:06

Copy link

Member

nicolas-grekas commentedJun 14, 2018

I rebased your branch on 3.4 and pushed it on your fork.
See adjusted patch.

Status: needs review

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Jun 14, 2018

nicolas-grekas added ProxyManagerBridge and removed DependencyInjection labels

Jun 14, 2018

Copy link

Member

nicolas-grekas commentedJun 15, 2018

Good catch, thanks@omnilight.

nicolas-grekas merged commit198bee0 intosymfony:3.4

Jun 15, 2018

nicolas-grekas added a commit that referenced this pull request

Jun 15, 2018

bug#27593[ProxyManagerBridge] Fixed support of private services (ni…

56f5d83

…colas-grekas)This PR was merged into the 3.4 branch.Discussion----------[ProxyManagerBridge] Fixed support of private services| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | #...| License       | MIT| Doc PR        |<!--Write a short README entry for your feature/bugfix here (replace this comment block.)This will help people understand your PR and can be used as a start of the Doc PR.Additionally: - Bug fixes must be submitted against the lowest branch where they apply   (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch.-->Fixed lazy loading of private services, that was broken since Symfony 4.0 release because of renamingaddObjectResourcehttps://github.com/symfony/symfony/blame/fa022f05be75aacc8afcdc11b63333685c719e13/src/Symfony/Component/DependencyInjection/CHANGELOG.md#L114Commits-------198bee0 [ProxyManagerBridge] Fixed support of private services

This was referencedJun 25, 2018

Release v3.4.12#27704

Merged

Release v4.0.12#27705

Merged

Release v4.1.1#27706

Merged

Copy link

Member

@nicolas-grekas this patch is broken, because$this->privates is not used to store private services in DI 3.4, only in 4.0+. So we still need a different logic in the ProxyManagerBridge based on the Symfony DI version.