8000 Container::set method description is wrong · Issue #30270 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Container::set method description is wrong #30270

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.

Sign up for GitHub

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

Jump to bottom
Closed
speller opened this issue Feb 16, 2019 · 6 comments
Closed

Container::set method description is wrong #30270

speller opened this issue Feb 16, 2019 · 6 comments
Labels
DependencyInjection

Comments

@speller
Copy link
speller commented Feb 16, 2019

Symfony version(s) affected: 4.2

Description
The Symfony\Component\DependencyInjection\Container::set annotation states the following:

Setting a service to null resets the service: has() returns false and get()
behaves in the same way as if the service was never created.

But this is impossible to do because only synthetic services can be set to null. It is impossible to dispose a regular public or private service instance when using with app servers like PHP RoadRunner which doesn't finish scripts. In this case DB connections defined as regular services are not disposed and quickly wastes DB max connections.

Possible Solution
Allow to null any service.
Or define and document a legal way to do that.
@stof
Copy link
Member
stof commented Feb 18, 2019

I think the phpdoc was not updated properly when deprecating that behavior.

Or define and document a legal way to do that.

There is a reset method on the container, which resets all instances (resetting only a single instance does not work to release, as it might still be referenced by other instantiated services depending on it).

@speller
Copy link
Author
speller commented Feb 18, 2019

Thank you for the answer. What about adding the reset method to the Symfony\Component\DependencyInjection\ContainerInterface?

@ro0NL
Copy link
Contributor
ro0NL commented Feb 18, 2019

@speller it's in ResettableContainerInterface already :)

@speller
Copy link
Author
speller commented Feb 18, 2019

Yes, the only addition is:

  • @deprecated since Symfony 4.2, use "Symfony\Contracts\Service\ResetInterface" instead.

Also, I've found I have two files implementing this interface: c:\...\vendor\symfony\contracts\Service\ResetInterface.php and c:\...\vendor\symfony\symfony\src\Symfony\Contracts\Service\ResetInterface.php (under Windows). Is it correct that files are duplicated?

@ro0NL
Copy link
Contributor
ro0NL commented Feb 18, 2019

oh, oops. It's ResetInterface now yes.

Is it correct that files are duplicated?

Yes, symfony/contracts is a standalone composer package, but split from this repo. When autoloading it will be used from the first location.

@nicolas-grekas
Copy link
Member

PR welcome: we should add the "synthetic" word at the beginning to improve that, isn't it?
Setting a synthetic service [...]
The PR should be submitted on branch 3.4, because that's the only non-deprecated behavior there also.

@xabbuh xabbuh mentioned this issue Mar 8, 2019
Merged
@fabpot fabpot closed this as completed Mar 9, 2019
fabpot added a commit that referenced this issue Mar 9, 2019
@fabpot
minor #30495 [DependencyInjection] update docblock to match the actua…
7d227e9 
…l behavior (xabbuh)

This PR was merged into the 3.4 branch.

Discussion
----------

[DependencyInjection] update docblock to match the actual behavior

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30270
| License       | MIT
| Doc PR        |

Commits
-------

83826da update docblock to match the actual behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DependencyInjection
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @nicolas-grekas @stof @ro0NL @speller @chalasr
0