NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[2.3] Handle PHP sessions started outside of Symfony#7571

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

ghost wants to merge5 commits intosymfony:masterfromunknown repository

Closed

[2.3] Handle PHP sessions started outside of Symfony#7571

ghost wants to merge5 commits intosymfony:masterfromunknown repository

Conversation

Copy link

ghost commentedApr 5, 2013

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

This PR brings a way to allow Symfony2 to manage a session started outside of Symfony in such a way that quite explicit. It also introduces more robust detection of previously started sessions under PHP 5.3 and supports real session status detection under PHP 5.4

bendavies reviewed

Apr 5, 2013

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php Outdated

Copy link

Contributor

bendaviesApr 5, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

not need to doversion_compare twice?

Copy link

Author

ghostApr 5, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

My logic is the second check should not be madeat all if the first fails due to session not started. PHP version is required here because the firstif is precise under PHP 5.4.

[HttpFoundation][Session] Better detection of existing sessions.

9bf391f

Extended and improved tests, and introduced a way to work with applicationsthat start the PHP session using session_start()

Copy link

Contributor

bamarni commentedApr 5, 2013

great feature!

Is the metadatabag appropriate for the PhpSessionStorage? As the session may be created and updated outside Symfony, information contained in it might be wrong.
Imo the clear() method should be overrided in PhpSessionStorage, and it should only clear keys managed by Symfony instead of clearing the whole $_SESSION global, as Symfony can't work with what has been created out of the box (it has its own namespace), why should it remove it? To me this feature is only about sharing the storage driver, not the data.

[HttpFoundation][Session] Don't wipe out non-Symfony session keys for…

3fcceae

… PhpSessionStorage.

Copy link

Author

ghost commentedApr 5, 2013

@bamarni - point 1 - the purpose of this is to allow Symfony to interface with a system where you cannot avoid it starting the session. This allows legacy code to do it's thing unhindered and for Symfony do manage it's stuff. The chance of a key collision is of course possible though remote, but the chance of the legacy code to overwrite the entire$_SESSION is always there - that's why I've been so vehemently opposed to doing anything like this (there are many other things that can go wrong too). But doing things this way is explicit, so I feel it gives the programmer plenty awareness of what they are doing. Together with the documentation, I think it's usecase will be clear.

As for 2, that was well spotted. I will make the change now.