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

[HttpFoundation] Allow reuse of Session between requests if ID did not change #28433

Merged
merged 1 commit into from Sep 21, 2018

Conversation

Projects
None yet
4 participants
@tgalopin
Copy link
Member

commented Sep 10, 2018

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #13450
License MIT
Doc PR -

I stumbled upon the issue from #13450 in a more simple case than what was exposed in the issue. From my understanding, the problem arises when the session is used between an access to the session and a functional test request: because the session was accessed (usually using the container directly), the session has started and the following request fails.

This PR checks whether the ID was actually regenerated before throwing (if a setId is called with the same ID, it is the same request context, it shouldn't throw IMO).

Not sure I understood everything correctly though, feel free to fix it for me if needed.

@nicolas-grekas
Copy link
Member

left a comment

Would you mind adding a test case please?
Otherwise, lgtm thanks.

@tgalopin tgalopin force-pushed the tgalopin:patch-2 branch from ded4ca3 to 8f60a8a Sep 11, 2018

$this->storage->setId($id);
} catch (\Exception $e) {
$exception = $e;
}

This comment has been minimized.

Copy link
@chalasr

chalasr Sep 11, 2018

Member
$e = null;
try {
    $this->storage->setId($id);
} catch (\Exception $e) {
}
$this->assertNull($e);

same below

@@ -97,6 +97,30 @@ public function testGetId()
$this->assertNotEquals('', $this->storage->getId());
}
public function testSetId()

This comment has been minimized.

Copy link
@chalasr

chalasr Sep 11, 2018

Member

testSetIdAfterStart()?

$this->assertNull($exception);
$exception = null;

This comment has been minimized.

Copy link
@chalasr

chalasr Sep 11, 2018

Member

no need to reassign

@chalasr chalasr added this to the 2.8 milestone Sep 11, 2018

@tgalopin tgalopin force-pushed the tgalopin:patch-2 branch from 8f60a8a to 6ea3f26 Sep 13, 2018

@nicolas-grekas nicolas-grekas changed the title Allow reuse of MockArraySessionStorage between requests [HttpFoundation] Allow reuse of MockArraySessionStorage between requests Sep 17, 2018

@tgalopin tgalopin force-pushed the tgalopin:patch-2 branch from 6ea3f26 to e6422cb Sep 20, 2018

@tgalopin tgalopin changed the title [HttpFoundation] Allow reuse of MockArraySessionStorage between requests [HttpFoundation] Allow reuse of Session between requests if ID did not change Sep 20, 2018

@tgalopin tgalopin force-pushed the tgalopin:patch-2 branch from e6422cb to fd30f4a Sep 21, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

Thank you @tgalopin.

@nicolas-grekas nicolas-grekas merged commit fd30f4a into symfony:2.8 Sep 21, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Sep 21, 2018

bug #28433 [HttpFoundation] Allow reuse of Session between requests i…
…f ID did not change (tgalopin)

This PR was merged into the 2.8 branch.

Discussion
----------

[HttpFoundation] Allow reuse of Session between requests if ID did not change

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #13450
| License       | MIT
| Doc PR        | -

I stumbled upon the issue from #13450 in a more simple case than what was exposed in the issue. From my understanding, the problem arises when the session is used between an access to the session and a functional test request: because the session was accessed (usually using the container directly), the session has started and the following request fails.

This PR checks whether the ID was actually regenerated before throwing (if a setId is called with the same ID, it is the same request context, it shouldn't throw IMO).

Not sure I understood everything correctly though, feel free to fix it for me if needed.

Commits
-------

fd30f4a Allow reuse of Session between requests

@tgalopin tgalopin deleted the tgalopin:patch-2 branch Sep 21, 2018

This was referenced Sep 30, 2018

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.