Skip to content
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

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

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
7 participants
@ghost
Copy link

commented Apr 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

View changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php Outdated
return true;
if (version_compare(phpversion(), '5.4.0', '>=') && session_status() === \PHP_SESSION_ACTIVE) {
throw new \RuntimeException('Failed to start the session: already started by PHP');
} elseif (version_compare(phpversion(), '5.4.0', '<') && isset($_SESSION) && session_id()) {

This comment has been minimized.

Copy link
@bendavies

bendavies Apr 5, 2013

Contributor

not need to do version_compare twice?

This comment has been minimized.

Copy link
@ghost

ghost Apr 5, 2013

Author

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

@bendavies

View changes

src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php Outdated
@@ -72,16 +72,31 @@ public function isWrapper()
*/
public function isActive()
{
if (version_compare(phpversion(), '5.4.0', '>=')) {
return $this->active = session_status() === \PHP_SESSION_ACTIVE ? true : false;

This comment has been minimized.

Copy link
@bendavies

bendavies Apr 5, 2013

Contributor

? true : false is redundant

This comment has been minimized.

Copy link
@ghost

ghost Apr 5, 2013

Author

fixed.

@pborreli

View changes

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/PhpSessionStorageTest.php Outdated
$this->assertTrue(isset($_SESSION));
// in PHP 5.4 we can reliably detect a session started
$this->assertTrue($storage->getSaveHandler()->isActive());
// PHP sesion might have started, but the storage driver has not, so false is correct here

This comment has been minimized.

Copy link
@pborreli

pborreli Apr 5, 2013

Contributor

typo sesion => session

Drak
[HttpFoundation][Session] Better detection of existing sessions.
Extended and improved tests, and introduced a way to work with applications
that start the PHP session using session_start()
@bamarni

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2013

great feature!

  1. Is the metadatabag appropriate for the PhpSessionStorage? As the session may be created and updated outside Symfony, information contained in it might be wrong.

  2. 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.

Drak
@ghost

This comment has been minimized.

Copy link
Author

commented Apr 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.

if (!$saveHandler instanceof AbstractProxy &&
!$saveHandler instanceof NativeSessionHandler &&
!$saveHandler instanceof \SessionHandlerInterface &&
$saveHandler !== null) {

This comment has been minimized.

Copy link
@bendavies

bendavies Apr 5, 2013

Contributor

symfony standard is usually the reverse of this?
null !== $saveHandler

@stanlemon

This comment has been minimized.

Copy link

commented Apr 5, 2013

This would be a great addition, it would make my life introducing Symfony into legacy code a lot easier.

@stof

View changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php Outdated
return true;
if (version_compare(phpversion(), '5.4.0', '>=') && \PHP_SESSION_ACTIVE === session_status()) {
throw new \RuntimeException('Failed to start the session: already started by PHP');
} elseif (version_compare(phpversion(), '5.4.0', '<') && isset($_SESSION) && session_id()) {

This comment has been minimized.

Copy link
@stof

stof Apr 5, 2013

Member

This can be if instead of elseif as the previous if block throws an exception

@stof

View changes

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

if (!$this->saveHandler->isWrapper() && !$this->saveHandler->isSessionHandlerInterface()) {
$this->saveHandler->setActive(false);
if (!$this->saveHandler->isWrapper() && !$this->getSaveHandler()->isSessionHandlerInterface()) {

This comment has been minimized.

Copy link
@stof

stof Apr 5, 2013

Member

why changing it to use the getter the second time ?

This comment has been minimized.

Copy link
@ghost

ghost Apr 5, 2013

Author

Man you have good eyes :) fixed.

@ghost

This comment has been minimized.

Copy link
Author

commented Apr 6, 2013

@fabpot - is this PR acceptable now before I start on the docs?

@fabpot

View changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php Outdated
throw new \RuntimeException('Failed to start the session: already started by PHP');
} elseif (version_compare(phpversion(), '5.4.0', '<') && isset($_SESSION) && session_id()) {
// not 100% fool-proof, but is the most reliable way to determine if a session is active in PHP 5.3
throw new \RuntimeException('Failed to start the session: already started by PHP ($_SESSION is set)');

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 6, 2013

Member

All exception messages must end with a dot.

This comment has been minimized.

Copy link
@ghost

ghost Apr 6, 2013

Author

Fixed.

@ghost ghost referenced this pull request Apr 6, 2013

Merged

[WCM] Document PhpSessionStorage #2474

@ghost

This comment has been minimized.

Copy link
Author

commented Apr 6, 2013

Updated with docs PR.

@staabm

View changes

src/Symfony/Component/HttpFoundation/Session/Storage/PhpSessionStorage.php Outdated
}

$this->loadSession();
if (!$this->saveHandler->isWrapper() && !$this->getSaveHandler()->isSessionHandlerInterface()) {

This comment has been minimized.

Copy link
@staabm

staabm Apr 7, 2013

Contributor

Here also getter at 2nd condition

This comment has been minimized.

Copy link
@ghost

ghost Apr 7, 2013

Author

Fixed.

@staabm

View changes

src/Symfony/Component/HttpFoundation/Session/Storage/PhpSessionStorage.php Outdated
/**
* Constructor.
*
* @param object|null $handler Must be instance of AbstractProxy or NativeSessionHandler;

This comment has been minimized.

Copy link
@staabm

staabm Apr 7, 2013

Contributor

Phpdoc allows those type information also here @param AbstractProxy|NativeSessionHandler|SessionHandlerInterface|null

This comment has been minimized.

Copy link
@ghost

ghost Apr 7, 2013

Author

It looks really unweildly like that:

    /**
     * Constructor.
     *
     * @param AbstractProxy|NativeSessionHandler|\SessionHandlerInterface|null $handler
     * @param MetadataBag                                                      $metaBag MetadataBag.
     */
    public function __construct($handler = null, MetadataBag $metaBag = null)

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 7, 2013

Member

Looks good to me.

This comment has been minimized.

Copy link
@ghost

ghost Apr 7, 2013

Author

OK, done.

$this->assertFalse($storage->getSaveHandler()->isActive());
$this->assertFalse($storage->isStarted());

session_start();

This comment has been minimized.

Copy link
@staabm

staabm Apr 7, 2013

Contributor

Should the session be destroyed so no sideffect are left behind by this test

This comment has been minimized.

Copy link
@ghost

ghost Apr 7, 2013

Author

Yes. I had meant to do that but forgot. Adding now.

@staabm

View changes

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/PhpSessionStorageTest.php Outdated
class PhpSessionStorageTest extends \PHPUnit_Framework_TestCase
{
/**
* @param array $options

This comment has been minimized.

Copy link
@staabm

staabm Apr 7, 2013

Contributor

There is no param

This comment has been minimized.

Copy link
@ghost

ghost Apr 7, 2013

Author

Fixed.

@fabpot fabpot referenced this pull request Apr 9, 2013

Closed

Update session start check #7216

@fabpot

View changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php Outdated
&& $this->saveHandler->isSessionHandlerInterface()) {
$this->loadSession();
if (version_compare(phpversion(), '5.4.0', '>=') && \PHP_SESSION_ACTIVE === session_status()) {
throw new \RuntimeException('Failed to start the session: already started by PHP');

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 11, 2013

Member

There is some missing dots at the end of exception messages (and you removed some as well).

This comment has been minimized.

Copy link
@ghost

ghost Apr 11, 2013

Author

The CS is mainly sorted out in the other PR. I'll fix this particular one though.

This comment has been minimized.

Copy link
@ghost

ghost Apr 11, 2013

Author

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.