Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Update src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php #4456

Merged
merged 1 commit into from

4 participants

@guilhermeblanco

Better patch for initialization/regeneration of id of already started session

@travisbot

This pull request passes (merged 3ef377b into 223d187).

@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch guilhermeblanco/patch-6 (PR #4456)
Commits
-------

3ef377b Update src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php

Discussion
----------

Update src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php

Better patch for initialization/regeneration of id of already started session

---------------------------------------------------------------------------

by travisbot at 2012-05-30T05:31:15Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1474248) (merged 3ef377b into 223d187).
6c527b6
@fabpot fabpot merged commit 3ef377b into symfony:master
@ghost

@guilhermeblanco - With this, can the related tickets can be closed now?

@guilhermeblanco

@drak not yet.
I will work on a test case for this, because it seems it's possible to destroy=true.

@ghost

I dont get why you are using regenerate here, instead of the previously proposed patch of simply doing nothing - or was there an issue with that?

@mevers47

This is causing my code to break. I got a security firewall listener that relies on a session which isn't started yet so it can call $session->setId(). I do this because we use a custom authentication mechanism that does not rely on cookies, a cookie-less session. Therefore I agree with @drak , if the previous patch didn't had an issue we shouldn't regenerate.

@ghost

Something is still suspicious about this patch. There was never an explanation given.

@mevers47

This patch interferes with our integration tests. If the purpose of this listener is to emulate cookie handling as PHP does normally with its request (read session cookie) > respond (write session cookie, only when the cookie is not set before) then this patch introduces a bug.

Assuming session.auto_start is off, when a plain request comes in you can set the session id without trouble. This listener should emulate and wrap session handling as if a plain request was send and set a cookie like PHP normally does with its response. Therefore allowing the session id to be set.

Although PHP sets the session cookie only the first time (ie. when no session cookie was present in the request), setting it in every response may not cause any trouble.

@guilhermeblanco

@drak the issue is caused when you want to emulate an authentication by starting your own session and "authentication" a user during a test case.
If you don't migrate a previously created session, Symfony creates a new one (not authenticated) and your test will always fail. That piece of code was fixing this issue.

Recently I tracked down this issue again (I also didn't like this patch) and I could actually figure it out that at weird scenarios, my session was being created with no id. This was actually interfering with the Symfony session, forcing the regeneration, loosing the emulated authentication. To fix that, I forced to always inject uniqid() as my session id and everything was solved.
After that, I submitted the PR to http://github.com/liip/LiipFunctionalTestBundle which was merged a while ago: liip/LiipFunctionalTestBundle#55

So this patch could be reverted, since the issue could be solved another way.

@fabpot
Owner

reverted: 826f512

@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot Revert "merged branch guilhermeblanco/patch-6 (PR #4456)"
This reverts commit 6c527b6, reversing
changes made to 223d187.
826f512
@dustinwhittle dustinwhittle referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ramsey ramsey referenced this pull request from a commit
Ben Ramsey Merge remote-tracking branch 'upstream/master'
* upstream/master: (201 commits)
  Update src/Symfony/Bundle/FrameworkBundle/composer.json
  small optimization
  fixes pre for var_dump with xdebug
  [FrameworkBundle] Allow to set null for the handler in NativeSessionStorage
  added a missing phpdoc param
  [HttpFoundation] fixed undefined offset for assoc arrays in HeaderBag
  [HttpFoundation] fix #5271 (duplicated header in JsonResponse)
  fixed typos in the UPGRADE file
  fixed typo in the UPGRADE file
  [Form] Reintroduced the option "invalid_message_parameters"
  [Form] Fixed support for preferred choices in "entity" type
  Typo in UPGRADE-2.1
  [EventDispatcher] Adding IteratorAggregate to GenericEvent
  fix CS into Finder
  0x02 -> \MongoBinData::BYTE_ARRAY
  Alter upgrade notes with changes to _form_is_choice_selected twig function
  MongoBinData constructor now require "type" parameter
  Revert "merged branch guilhermeblanco/patch-6 (PR #4456)"
  add format to exception message
  Adding more specific debug bar reset CSS.
  ...
5bb89b6
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch guilhermeblanco/patch-6 (PR #4456)
Commits
-------

3ef377b Update src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php

Discussion
----------

Update src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php

Better patch for initialization/regeneration of id of already started session

---------------------------------------------------------------------------

by travisbot at 2012-05-30T05:31:15Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1474248) (merged 3ef377b into 223d187).
6ba3af0
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot Revert "merged branch guilhermeblanco/patch-6 (PR #4456)"
This reverts commit 6c527b6, reversing
changes made to 223d187.
9819cfd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  src/Symfony/Bundle/FrameworkBundle/EventListener/TestSessionListener.php
@@ -52,6 +52,8 @@ public function onKernelRequest(GetResponseEvent $event)
if ($cookies->has($session->getName())) {
$session->setId($cookies->get($session->getName()));
+ } else {
+ $session->migrate(false);
}
}
Something went wrong with that request. Please try again.