Skip to content

[HttpKernel] Move required RequestStack args as first arguments #15724

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

Merged
merged 1 commit into from
Sep 10, 2015

Conversation

nicolas-grekas
Copy link
Member

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

Since we planned to make RequestStack required, we have to move it as first arguments.

@nicolas-grekas
Copy link
Member Author

deps=high failures will be fixed once this PR is merged into master

@Tobion
Copy link
Contributor

Tobion commented Sep 8, 2015

See #15196

@nicolas-grekas nicolas-grekas force-pushed the req-stack branch 2 times, most recently from 819c5e8 to d106c1b Compare September 8, 2015 11:03
@fabpot
Copy link
Member

fabpot commented Sep 9, 2015

I still don't get why we should make such changes. It would make impossible/very hard to support all Symfony versions in Silex for instance.

@nicolas-grekas
Copy link
Member Author

@fabpot because we have these comments that say: RequestStack will become required in 3.0., which implies that it is moved as first arguments.
For Silex, you could simply check the Kernel::VERSION_ID consts don't you think?

@fabpot
Copy link
Member

fabpot commented Sep 10, 2015

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 84ba05b into symfony:2.8 Sep 10, 2015
fabpot added a commit that referenced this pull request Sep 10, 2015
…arguments (nicolas-grekas)

This PR was merged into the 2.8 branch.

Discussion
----------

[HttpKernel] Move required RequestStack args as first arguments

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

Since we planned to make RequestStack required, we have to move it as first arguments.

Commits
-------

84ba05b [HttpKernel] Move required RequestStack args as first arguments
@nicolas-grekas nicolas-grekas deleted the req-stack branch September 10, 2015 07:48
@xabbuh
Copy link
Member

xabbuh commented Sep 25, 2015

We should document this in the upgrade file for Symfony 2.8.

@fabpot fabpot mentioned this pull request Nov 16, 2015
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.

5 participants