E532 [FrameworkBundle][HttpFoundation][Security] Deprecate service "session" by jderusse · Pull Request #38616 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle][HttpFoundation][Security] Deprecate service "session" #38616

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
Merged
merged 1 commit into from
Jan 28, 2021
Merged

[FrameworkBundle][HttpFoundation][Security] Deprecate service "session" #38616

merged 1 commit into from
Jan 28, 2021

Conversation

@jderusse
Copy link
Member
@jderusse jderusse commented Oct 17, 2020 •
edited
Loading
Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? yes
Tickets Fix #10557 and Fix #12839
License MIT
Doc PR TODO

This is a attempt to deprecate service session and SessionInterface.

This PR replaces the session service by a .session.do-not-use service (used internally by Symfony) and make session a deprecated alias.
In Symfony 6.0 we can remove the session service and replace the SessionListener by a Factory that build the session (instead of fetching it from container)

This PR also add a short cut RequestStack::getSession(): ?SessionInterface

For backward compatibility the SessionListener is replaced by FactorySessionListener only when the user don't override the service session (ping @wouterj )

TODO:

  • Test many configuration and dependencies (ie. session disabled + csrf)
  • ChangeLog and Upgrade
  • fix tests

jderusse
jderusse commented Oct 17, 2020
View reviewed changes
@jderusse jderusse marked this pull request as draft October 17, 2020 21:38
@jderusse jderusse force-pushed the session-deprecat branch 4 times, most recently from 6ccd212 to dea1e7e Compare October 18, 2020 06:18
@chalasr chalasr changed the title [FrameworkBundle] Deprecat service "session" [FrameworkBundle] Deprecate service "session" Oct 18, 2020
@jderusse
Copy link
Member Author
jderusse commented Oct 18, 2020

hmmm what's about renaming service session into .sesion, and create a deprecated alias sesion pointing to .session

In that way session is deprecated. but still overridable or injectable without drawback.

internal services will inject the .session service to not trigger deprecation.

@ro0NL
Copy link
Contributor
ro0NL commented Oct 19, 2020

internal services should be refactored IMHO to use the new approach :) if the session service itself is deprecated, no need for an alias id say.

@jderusse
Copy link
Member Author
jderusse commented Oct 19, 2020

if the session service itself is deprecated, no need for an alias id say.

issue is, we can't register 2 Session instances at the same time.
If we deprecate session, it can't be used internally (otherwise we'll trigger deprecation). That's why I setup the new FactorySessionListener.
BUT if people still use the session service (ie. injecting in the app's services) the application will break (because 2 distinguished instance of sessions are created by both the App and Symfony Internally).

A solution would be to check if the service is used to decide if the application should keep the old deprecated session service or if symfony internal's services should use the new mechanism. But checking if a service is used sounds very complex.

@ro0NL
Copy link
Contributor
ro0NL commented Oct 19, 2020 •
edited
Loading

it can't be used internally

and shouldnt also :)

cant we create a deprecated service factory instead? session: { factory: ['@request_stack', 'getSession'] }

@ro0NL
Copy link
Contributor
ro0NL commented Oct 19, 2020 •
edited
Loading

Ah, i figured. If the user created an overridden/independent session service, we need to account for it still.

Sounds like we need a check a la if ($this->sessionService !== $requestStack->getSession()) { /* deprecated case */ }, but im not sure this will trigger a depraction for the deprecated service, if so we need an intermediary .session service i agree.

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Oct 19, 2020
@jderusse jderusse force-pushed the session-deprecat branch 8 times, most recently from 79d1158 to 6584d06 Compare October 19, 2020 21:42
@jderusse jderusse force-pushed the session-deprecat branch 4 times, most recently from bbe492d to 1670747 Compare October 20, 2020 15:17
Tobion
Tobion reviewed Jan 10, 2021
View reviewed changes
@jderusse jderusse force-pushed the session-deprecat branch 5 times, most recently from d58f8b2 to b9ba274 Compare January 15, 2021 13:32
@jderusse jderusse force-pushed the session-deprecat branch 2 times, most recently from caec7c1 to 6d608e4 Compare January 15, 2021 13:54
nicolas-grekas
nicolas-grekas reviewed Jan 15, 2021
View reviewed changes
@jderusse jderusse force-pushed the session-deprecat branch 4 times, most recently from fba5556 to b8d9d91 Compare January 18, 2021 23:32
nicolas-grekas
nicolas-grekas reviewed Jan 21, 2021
View reviewed changes
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 28, 2021

Thank you @jderusse.

@nicolas-grekas nicolas-grekas mentioned this pull request Jan 28, 2021
Closed
@nicolas-grekas nicolas-grekas merged commit f0c3bc9 into symfony:5.x Jan 28, 2021
@jderusse jderusse deleted the session-deprecat branch January 28, 2021 22:02
@javiereguiluz javiereguiluz mentioned this pull request Jan 29, 2021
Closed
@weaverryan weaverryan mentioned this pull request Feb 2, 2021
Merged
@ro0NL ro0NL mentioned this pull request Feb 6, 2021
Closed
@jrushlow jrushlow mentioned this pull request Feb 16, 2021
Closed
@wouterj wouterj mentioned this pull request Apr 12, 2021
Closed
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
@dborsatto dborsatto mentioned this pull request May 6, 2021
528D Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wouterj wouterj wouterj left review comments

@derrabus derrabus derrabus left review comments

@chalasr chalasr chalasr left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

+2 more reviewers

@Tobion Tobion Tobion left review comments

@ro0NL ro0NL ro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Deprecation FrameworkBundle HttpFoundation Security Status: Reviewed

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

[HttpFoundation] Move Session Support Away [5.0][HttpFoundation] The session must not be a service

10 participants

@jderusse @ro0NL @nicolas-grekas @xabbuh @fabpot @Tobion @wouterj @derrabus @chalasr @carsonbot
0