Skip to content
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

[DependencyInjection] Skip empty proxy code #28861

Merged
merged 1 commit into from Oct 20, 2018

Conversation

olvlvl
Copy link
Contributor

@olvlvl olvlvl commented Oct 14, 2018

Q A
Branch? 4.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28852
License MIT

Fix #28852

@nicolas-grekas I'm not sure which branch this should be applied to, please let me know.

@olvlvl
Copy link
Contributor Author

olvlvl commented Oct 14, 2018

Looks like there's an issue during the php7.2 build.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 14, 2018

Note that while the code may not work correctly when no code is returned, I'm not sure it's legit to return nothing:
looking at your work in https://github.com/olvlvl/symfony-dependency-injection-proxy, I see that you generate proxies as anonymous classes. But this means that if the same interface (or set of interfaces) is proxied several times for several different services, the implementation for the proxy is going to be duplicated as many times.
Might be better to consider returning nothing is unsupported and dedup proxies on your side?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 17, 2018
@nicolas-grekas nicolas-grekas changed the base branch from master to 2.8 October 20, 2018 20:16
@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 2.8 Oct 20, 2018
@nicolas-grekas
Copy link
Member

Thank you @olvlvl.

@nicolas-grekas nicolas-grekas merged commit baf6f8c into symfony:2.8 Oct 20, 2018
nicolas-grekas added a commit that referenced this pull request Oct 20, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

[DependencyInjection] Skip empty proxy code

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28852
| License       | MIT

Fix #28852

@nicolas-grekas I'm not sure which branch this should be applied to, please let me know.

Commits
-------

baf6f8c Skip empty proxy code
@olvlvl olvlvl deleted the olvlvl-phpdumper branch October 22, 2018 08:08
This was referenced Nov 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants