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

[DI] 3.4 BC Break - all my services made private #24465

Closed
Strate opened this Issue Oct 6, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@Strate
Contributor

Strate commented Oct 6, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no
Symfony version 3.4.x-dev

I tried to upgrade from 3.3 to 3.4.x-dev to test compatibility and found that all of my services made private by default. It happes from Symfony\Component\DependencyInjection\Compiler\ResolveChildDefinitionsPass::process method:

foreach ($container->getDefinitions() as $definition) {
    if ($definition->isPrivate()) {
        $definition->setPublic(false);
        $definition->setPrivate(true);
    }
}

->isPrivate() method returns true for all services by default, because \Symfony\Component\DependencyInjection\Definition::$private set as true by default.

/cc @nicolas-grekas

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 6, 2017

Member

What the behavior you have? Any error message?

Member

nicolas-grekas commented Oct 6, 2017

What the behavior you have? Any error message?

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 6, 2017

Contributor

I have my custom CompilerPass which checks for isPublic and fails if it is falsy (this need for custom services collection by tags)

Contributor

Strate commented Oct 6, 2017

I have my custom CompilerPass which checks for isPublic and fails if it is falsy (this need for custom services collection by tags)

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 6, 2017

Member

@Strate 4.0 will make services private by default. If you don't specify the visibility of services, 3.4 will indeed make them semi-private in this case: it will treat them as private, but ensure that they cannot be removed or inlined, so that they are still accessible in Container::get() thanks to the BC layer.
Note that in most cases, new Symfony DI 3.3 features (especially IteratorArgument and ServiceLocator) make it possible to avoid the need to use public services for lazy references.

@nicolas-grekas should we do this change on setPublic at a lower priority than at the beginning of priority 0 compiler passes ? It is common for bundles to check for isPublic in their compiler passes, and this would indeed not work well here when relying on the BC layer.

Member

stof commented Oct 6, 2017

@Strate 4.0 will make services private by default. If you don't specify the visibility of services, 3.4 will indeed make them semi-private in this case: it will treat them as private, but ensure that they cannot be removed or inlined, so that they are still accessible in Container::get() thanks to the BC layer.
Note that in most cases, new Symfony DI 3.3 features (especially IteratorArgument and ServiceLocator) make it possible to avoid the need to use public services for lazy references.

@nicolas-grekas should we do this change on setPublic at a lower priority than at the beginning of priority 0 compiler passes ? It is common for bundles to check for isPublic in their compiler passes, and this would indeed not work well here when relying on the BC layer.

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 6, 2017

Contributor

@stof thank you, I've got a reason why it was done. I will definitely refactor my code for sf4

Contributor

Strate commented Oct 6, 2017

@stof thank you, I've got a reason why it was done. I will definitely refactor my code for sf4

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 6, 2017

Member

@Strate maybe you should run your pass BEFORE_OPTIMIZATION? It's strange to run a check at OPTIMIZATION step, isn't it?
@stof did you mean something like #24468?

Member

nicolas-grekas commented Oct 6, 2017

@Strate maybe you should run your pass BEFORE_OPTIMIZATION? It's strange to run a check at OPTIMIZATION step, isn't it?
@stof did you mean something like #24468?

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 6, 2017

Contributor

It's strange to run a check at OPTIMIZATION step, isn't it?

Yes, I think so too, but there is a reason to run it there: my compiler pass should run after ResolveDefinitionTemplates pass, and long time ago (when symfony was at version 2) I decided to just move my compiler pass to OPTIMIZATION step.

Contributor

Strate commented Oct 6, 2017

It's strange to run a check at OPTIMIZATION step, isn't it?

Yes, I think so too, but there is a reason to run it there: my compiler pass should run after ResolveDefinitionTemplates pass, and long time ago (when symfony was at version 2) I decided to just move my compiler pass to OPTIMIZATION step.

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 6, 2017

Contributor

@nicolas-grekas there is even such compiler pass in Symfony core: #20116

Contributor

Strate commented Oct 6, 2017

@nicolas-grekas there is even such compiler pass in Symfony core: #20116

fabpot added a commit that referenced this issue Oct 10, 2017

bug #24468 [DI] Turn private defs to non-public ones before removing …
…passes (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Turn private defs to non-public ones before removing passes

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

As spotted by @stof in #24465 (comment).

Commits
-------

e5d0934 [DI] Turn private defs to non-public ones before removing passes

symfony-splitter pushed a commit to symfony/dependency-injection that referenced this issue Oct 10, 2017

bug #24468 [DI] Turn private defs to non-public ones before removing …
…passes (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Turn private defs to non-public ones before removing passes

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

As spotted by @stof in symfony/symfony#24465 (comment).

Commits
-------

e5d0934b87 [DI] Turn private defs to non-public ones before removing passes

@fabpot fabpot closed this Oct 10, 2017

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