[HttpKernel] make RequestStack parameter required#15196
[HttpKernel] make RequestStack parameter required#15196fabpot merged 1 commit intosymfony:masterfrom
Conversation
7ed4189 to
6d6804e
Compare
There was a problem hiding this comment.
I don't understand why we need to change the order of arguments.
There was a problem hiding this comment.
Because having required parameters after optional ones (with defaults) makes no sense.
There was a problem hiding this comment.
But that works really well. As those classes are rarely used by the end user directly, I would avoid breaking BC here.
There was a problem hiding this comment.
If they are not used by end users, they would not be affected by the BC break since they rely on the service definition.
There was a problem hiding this comment.
I'm sure you got my point. I'm not going to argue forever as there is no reasons to break BC.
There was a problem hiding this comment.
I don't get it. There is a BC break anyway. In 2.x the RequestStack was optional. But in 3.0 it is required. The argument order is just a tiny detail. The BC break is there either way.
There was a problem hiding this comment.
The Bc break is that existing code passing the RequestStack already (the recommended usage) would now break
|
I understand why the RequestStack is moved in front of optional arguments, but this misses a continuous upgrade path. Making this PR 2.8 compatible with deprecations notices would at least prevent people from not noticing the change. |
|
@Tobion can you please rebase this PR? |
|
@Tobion Can you rebase this PR? |
|
👍 |
|
ping @symfony/deciders |
|
👍 |
There was a problem hiding this comment.
The order of the parameters was already changed for Symfony 2.8 and should therefore not be described here.
|
👍 |
|
We still need the rebase |
There was a problem hiding this comment.
this change is not true anymore AFAICT. 2.8 will still be compatible
|
@Tobion Can you rebase? |
6d6804e to
2047545
Compare
2047545 to
a2e154d
Compare
|
Rebased |
|
Thank you @Tobion. |
…ion) This PR was merged into the 3.0-dev branch. Discussion ---------- [HttpKernel] make RequestStack parameter required | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Continuation of #14634, #8904 Commits ------- a2e154d [HttpKernel] make RequestStack parameter required for classes that need it
Continuation of #14634, #8904