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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ProxyManagerBridge] Fixed support of private services #27593

Merged
merged 1 commit into from Jun 15, 2018

Conversation

Projects
None yet
8 participants
@omnilight

omnilight commented Jun 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

);
}
public function getPrivatePublicDefinitions()

This comment has been minimized.

@rodnaph

rodnaph Jun 13, 2018

Contributor

make static?

This comment has been minimized.

@scaytrase

scaytrase Jun 13, 2018

Contributor

what for? it's phpunit dataprovider

This comment has been minimized.

@rodnaph

rodnaph Jun 13, 2018

Contributor

avoids needing to create instance of test case to invoke

This comment has been minimized.

@rodnaph

rodnaph Jun 13, 2018

Contributor

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.

This comment has been minimized.

@omnilight

omnilight Jun 13, 2018

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

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

This comment has been minimized.

@jakzal

jakzal Jun 13, 2018

Member

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

This comment has been minimized.

@omnilight
/**
* @dataProvider getPrivatePublicDefinitions
*/
public function testCorrectPublicPrivateInstantiation(Definition $definition, string $access)

This comment has been minimized.

@jakzal

jakzal Jun 13, 2018

Member

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

This comment has been minimized.

@omnilight
@stof

This comment has been minimized.

Member

stof commented Jun 13, 2018

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

@stof

This comment has been minimized.

Member

stof commented Jun 13, 2018

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

@stof

This comment has been minimized.

Member

stof commented Jun 13, 2018

@nicolas-grekas can you confirm my diagnosis ?

@omnilight

This comment has been minimized.

omnilight commented Jun 13, 2018

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

@omnilight

This comment has been minimized.

omnilight commented Jun 13, 2018

And maybe check for the version...

@stof

This comment has been minimized.

Member

stof commented Jun 14, 2018

Well, this method_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.

@omnilight

This comment has been minimized.

omnilight commented Jun 14, 2018

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

@stof

This comment has been minimized.

Member

stof commented Jun 14, 2018

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

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jun 14, 2018

@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 Jun 14, 2018

@nicolas-grekas nicolas-grekas changed the title from [DI] Fixed support of private services since Symfony 4.0 to [ProxyManagerBridge] Fixed support of private services Jun 14, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 14, 2018

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

Status: needs review

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 15, 2018

Good catch, thanks @omnilight.

@nicolas-grekas nicolas-grekas merged commit 198bee0 into symfony:3.4 Jun 15, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 15, 2018

bug #27593 [ProxyManagerBridge] Fixed support of private services (ni…
…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 renaming
addObjectResource
https://github.com/symfony/symfony/blame/fa022f05be75aacc8afcdc11b63333685c719e13/src/Symfony/Component/DependencyInjection/CHANGELOG.md#L114

Commits
-------

198bee0 [ProxyManagerBridge] Fixed support of private services

This was referenced Jun 25, 2018

@stof

This comment has been minimized.

Member

stof commented Jun 29, 2018

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

@fabpot

This comment has been minimized.

Member

fabpot commented Jul 12, 2018

Shall we revert?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jul 12, 2018

I don't have the reference at hand, but it's already fixed.

@stof

This comment has been minimized.

Member

stof commented Jul 20, 2018

For reference, it is #27776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment