Skip to content

[Session] Fixed bug with mock file session storage #6342

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
Closed
wants to merge 1 commit into from
Closed

[Session] Fixed bug with mock file session storage #6342

wants to merge 1 commit into from

Conversation

baldurrensch
Copy link

Fixed a bug where an unstarted mock session would be emptied with a save. Here are the steps to reproduce:

  1. Use the test client from Symfony\Bundle\FrameworkBundle\Test\WebTestCase::createClient(), and add something to its session. (I actually had it authenticate against a firewall).
  2. Take the cookies of this first test client and add them to a second test client
  3. Have the second test client request a URL that results in a 404
  4. Since the 404 does not need to start the session, hence when save is called (automatically), the mock session is overwritten with an empty array. This does not happen with the other session handlers.

The added unit test in this PR shows this problem. If this PR gets accepted, will it also get merged into the 2.1.x-dev branch?

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes (The broken test seems to be unrelated to this change)
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -

[Session] Fixed bug with mock file session storage
2db8ecb 
Fixed a bug where an unstarted mock session would be emptied with a save
@fabpot
Copy link
Member

fabpot commented Dec 14, 2012

ping @Drak

@ghost
Copy link

ghost commented Dec 14, 2012

I think the logical reasoning is right because there a real unstarted session wouldn't call any save-handlers... but, what disturbs me is that the framework should not be calling the save() method if the session is not started. Maybe it doesn't matter where the stop is done, in the MockFileSessionStorage or by using a $session->isStarted() check in FrameworkBundle\Listener\TestSessionListener where it saves during testing. Can you tell me, does it also solve the problem if you just place if ($session = $event->getRequest()->getSession() && $session->isStarted()) { in the onKernelResponse method (and remove the other stop).

It's possible that a complete fix should actually include both checks, although I would suggest an exception be thrown in MockFileSessionStorage because something unexpected is happening and you should be notified about it.

What do you think? Let me know the result of the test suggested above.

(Regarding the branch question - bug fixes for the session should be branched from and applied to the 2.1 branch because it's a bug fix. Merges are applied up from the lowest branch affected. You have applied a branch to master which is for features or agreed BC breaks only).

@baldurrensch
Copy link
Author

Ok. I tested @Drak's suggestion, and it does indeed fix the problem. Here is what I will do:

  • Make the fix as suggested
  • throw an exception (RuntimeException) in the MockFileSessionStorage if the session is not started and save() is called.
  • Base it off of the 2.1 branch
  • Close this PR and open a new one.

How does that sound?

@fabpot
Copy link
Member

fabpot commented Dec 14, 2012

@baldurrensch Sounds good to me.

@ghost
Copy link

ghost commented Dec 14, 2012

Yes submit a new PR.

@baldurrensch baldurrensch mentioned this pull request Dec 14, 2012
Merged
@baldurrensch
Copy link
Author

I opened a new PR against the correct branch: #6362

@baldurrensch baldurrensch deleted the mocksession_save branch August 29, 2013 01:40
ondrejmirtes pushed a commit to ondrejmirtes/symfony that referenced this pull request Nov 25, 2013
@fabpot
merged branch baldurrensch/testlistener_fix (PR symfony#6362)
c67ddb2 
This PR was merged into the 2.1 branch.

Commits
-------

098b593 [Session] Added exception to save method
6b9ee87 [Session] Fixed a bug with the TestListener

Discussion
----------

[Session] Fixed bug with TestListener

Fixed a bug where an unstarted mock session would be emptied with a save. Here are the steps to reproduce:

Use the test client from Symfony\Bundle\FrameworkBundle\Test\WebTestCase::createClient(), and add something to its session. (I actually had it authenticate against a firewall).
Take the cookies of this first test client and add them to a second test client
Have the second test client request a URL that results in a 404
Since the 404 does not need to start the session, hence when save is called (automatically), the mock session is overwritten with an empty array. This does not happen with the other session handlers.
The added unit test in this PR shows this problem. If this PR gets accepted, will it also get merged into the 2.1.x-dev branch?

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes (The broken test seems to be unrelated to this change)
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -

This is a follow up PR since my original one (symfony#6342) was against the wrong upstream branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@baldurrensch @fabpot