[DI] Turn services and aliases private by default, with BC layer #24238
Conversation
32a7081 to
02fda81
Compare
| @@ -45,7 +46,7 @@ public function process(ContainerBuilder $container) | |||
| $aliases = array(); | |||
| foreach ($container->getAliases() as $name => $target) { | |||
| $this->currentId = $name; | |||
| $aliases[$this->bag->resolveValue($name)] = $this->bag->resolveValue($target); | |||
| $aliases[$this->bag->resolveValue($name)] = (new Alias($this->bag->resolveValue((string) $target)))->setPublic($target->isPublic())->setPrivate($target->isPrivate()); | |||
There was a problem hiding this comment.
was the old code actually resolving any parameter in aliases ? AFAIK, ParameterBag::resolveValue($object) is a no-op
There was a problem hiding this comment.
was a no-op yes, so that's a bug fix (but not one we have to do on lower branches, can be seen as a new feat now, isn't it?)
There was a problem hiding this comment.
well, I'm not sure we actually want to introduce this new feature here (especially not hidden inside a commit saying it turns aliases to private by default).
This should be discussed separately.
There was a problem hiding this comment.
Btw, I'm sure we had several discussions in the past rejecting the addition of this feature. I don't remember whether another discussion after that decided to add it (and the implementation failed at it)
There was a problem hiding this comment.
Looking at the history, this non-working code was added before the 2.0.0 release. So this is clearly before discussions where we rejected this feature.
So you should not add a rejected feature here. I would rather remove this no-op code from the codebase
There was a problem hiding this comment.
well, the key is still resolved, what should we do about it?
There was a problem hiding this comment.
Line reverted here as it's unrelated, I'll open another PR on 2.7 about this.
02fda81 to
8b1dfc5
Compare
|
(failure false-positive, known composer bug) |
| $container->register('my.type1', __CLASS__.'_Type1')->addTag('form.type'); | ||
| $container->register('my.type2', __CLASS__.'_Type2')->addTag('form.type'); | ||
| $container->setDefinition('form.extension', $extDefinition->setPublic(true)); | ||
| $container->register('my.type1', __CLASS__.'_Type1')->addTag('form.type')->setPublic(true); |
There was a problem hiding this comment.
Such changes in tests cases aren't always strictly required, right? In most cases the visibility does not matter in those test cases.
There was a problem hiding this comment.
Correct, I did it in batch to, more fine grained would need more time...
There was a problem hiding this comment.
Diff now reduced to the minimum.
6510d18 to
1fc59bc
Compare
| @@ -25,6 +25,7 @@ public function __construct($id, $public = true) | |||
| { | |||
| $this->id = (string) $id; | |||
| $this->public = $public; | |||
| $this->private = 2 > func_num_args(); | |||
There was a problem hiding this comment.
shouldn't this be $public && 2 > func_num_args() ?
If I register an alias as private explicitly, it does not need to have the BC layer keeping it from being inlined.
There was a problem hiding this comment.
when "private" is set to true, the BC layer is enabled and triggers deprecations,
so here, we want it to be "false" when either true or false is passes.
There was a problem hiding this comment.
hmm sorry, I misunderstood the condition (reading too fast).
2a04b39 to
57feec2
Compare
57feec2 to
9948b09
Compare
|
Now with UPGRADE/CHANGELOG entries. |
…h BC layer (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Turn services and aliases private by default, with BC layer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20048 | License | MIT | Doc PR | - With this PR, all services and aliases are made private by default. This is done in a BC way, thanks to the layer introduced in #24104. We will require bundles to explicitly opt-in for "public", either using "defaults", or stating `public="true"` explicitly. Same in DI extension, where calling `->setPublic(true)` will be required in 4.0. Commits ------- 9948b09 [DI] Turn services and aliases private by default, with BC layer
This PR was merged into the 3.4 branch. Discussion ---------- [DI] Fix private-by-default BC layer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24238 | License | MIT | Doc PR | - Spotted while merging 3.4 into master and by looking at the cross-versions CI. Commits ------- ff2ab58 [DI] Fix private-by-default BC layer
With this PR, all services and aliases are made private by default.
This is done in a BC way, thanks to the layer introduced in #24104.
We will require bundles to explicitly opt-in for "public", either using "defaults", or stating
public="true"explicitly. Same in DI extension, where calling->setPublic(true)will be required in 4.0.