8000 [WIP][HttpFoundation][Session] Fixed detection of an active session by dlsniper · Pull Request #4541 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WIP][HttpFoundation][Session] Fixed detection of an active session #4541

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
Merged
merged 1 commit into from
Jul 1, 2012
Merged

[WIP][HttpFoundation][Session] Fixed detection of an active session #4541

merged 1 commit into from
Jul 1, 2012

Conversation

dlsniper
Copy link
Contributor
@dlsniper dlsniper commented Jun 9, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: not sure
Symfony2 tests pass: no
Fixes the following tickets: #4529
Todo: Fix failing tests
License of the code: MIT
Documentation PR: ~

This fixes the problem when the session variable inside $request now has always data in it as it's now more powerful but this introduces the problem that the old way of detecting if a session is started or not doesn't work anymore.

@travisbot
Copy link
8000

This pull request passes (merged 9ae13e12 into 6266b72).

@ghost
Copy link
ghost commented Jun 10, 2012

Sessions should be started implicitly. The SF auto_start config parameter controls the session listener to start the session.

@dlsniper
Copy link
Contributor Author

So this patch is correct then and I should continue the work on it?

@ghost
Copy link
ghost commented Jun 11, 2012

@dlsniper - no it's not correct. The session should not be auto-started like this, @fabpot and I recently discussed it.

@dlsniper
Copy link
Contributor Author

@Drak, ok I'll remove the patch for auto_start then but the fix for start would still stand, right?

ghost
ghost reviewed Jun 12, 2012
View reviewed changes
src/Symfony/Component/HttpFoundation/Request.php
@@ -518,7 +518,7 @@ public function hasPreviousSession()
*/
public function hasSession()
{
return null !== $this->session;
return null !== $this->session && $this->session->isActive();
Copy link

Choose a reason for hiding this comment

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

I would call this isStarted()

@ghost
Copy link
ghost commented Jun 12, 2012

@dlsniper - I have no objection to the rest of the PR except for the autostart stuff. I've annotated for clarity :)

@travisbot
Copy link

This pull request fails (merged 3499980e into 37550d2).

@travisbot
Copy link

This pull request fails (merged dcc73071 into 37550d2).

@dlsniper
Copy link
Contributor Author

Seems Travis doesn't like the squashing of commits that I've did but the PR does pass the normal tests.
@Drak is this good for merging now?

Thanks :)

@dlsniper
Copy link
Contributor Author

@fabpot this can be merged safely, I've just applied the patch on my production application and the patch is ok, it's just travis failing.

Thanks

stloyd
stloyd reviewed Jun 13, 2012
View reviewed changes
src/Symfony/Component/HttpFoundation/Session/SessionInterface.php
@@ -175,6 +175,13 @@ function remove($name);
function clear();

/**
* Check if a session was started
*
* @return bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Boolean here and below, to match Sf2 CS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

@travisbot
Copy link

This pull request fails (merged 1a6eabd2 into 37550d2).

@travisbot
Copy link

This pull request fails (merged 4e3a93c8 into 37550d2).

@dlsniper
Copy link
Contributor Author

I've noticed that this is failing, I'll fix it later on today.

@travisbot
Copy link

This pull request fails (merged 5504c4b7 into 37550d2).

@ghost
Copy link
ghost commented Jun 13, 2012

It's possible that other tests are failing not related to this PR. Run the tests on the current master, and try rebasing your branch to the current master also.

@dlsniper
Copy link
Contributor Author

I've just reminded why this is failing on builds, I can't do them locally because of this:

Installing dev dependencies
Your requirements could not be solved to an installable set of packages.

        Problems:
                - Problem caused by:
                        - Installation request for doctrine/orm [>= 2.2.0.0, < 2.4.0.0-dev]: Satisfiable by [doctrine/orm-2.2.2, doctrine/orm-2.2.1, doctrine/orm-2.2.0, doctrine/orm-2.2.x-dev, doctrine/orm-2.3.x-dev].

I'll try and install this somehow and see what's wrong with it.

@mvrhov
Copy link
mvrhov commented Jun 13, 2012

@dlsniper: as @stof said to me this should be resolved in latest versions of composer, but it seems that is not. The problem is that composer cannot figure out that you are on dev-master if you try to instal dev. dependencies on feature branch. Take a look at the .travis.yml file on how to do a proper dev vendors install.
cc @Seldaek

@dlsniper
Copy link
Contributor Author

@mvrhov Thanks for pointing this out.

@Drak I still got two tests not passing but I'm not sure how to fix them as adding $session->start() will either fail with the message that the session has already been started, the headers_sent() call which returns true. Any help with them will be greatly appreciated. Thanks!

Here is what the HttpKernel tests are returning:

There were 2 failures:

1) Symfony\Component\HttpKernel\Tests\EventListener\LocaleListenerTest::testDefaultLocaleWithSession
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'es'
+'fr'

/var/www/symfony-orig/src/Symfony/Component/HttpKernel/Tests/EventListener/LocaleListenerTest.php:51

2) Symfony\Component\HttpKernel\Tests\EventListener\LocaleListenerTest::testLocaleFromRequestAttribute
Expectation failed for method name is equal to <string:set> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.


FAILURES!
Tests: 263, Assertions: 1025, Failures: 2, Skipped: 10.

@travisbot
Copy link

This pull request fails (merged 1004b7c0 into c07e916).

@travisbot
Copy link

This pull request fails (merged f72ba0a into c07e916).

@dlsniper
Copy link
Contributor Author

@stof / @vicb Hi, do either of you think that you can either point me out to the right direction for fixing this either ping someone else for home help as @Drak doesn't seem available for this and at the moment I'm pretty much clueless in what direction I should take this fix.

Thanks!

@dlsniper
Copy link
Contributor Author

ping @fabpot Can you please provide some input on this one as I'm a bit stuck and seems noone else is available.

@ghost
Copy link
ghost commented Jun 20, 2012

fyi - I'll be able to look again in a few days

fabpot
fabpot reviewed Jul 1, 2012
View reviewed changes
src/Symfony/Component/HttpFoundation/Request.php
@@ -518,7 +518,7 @@ public function hasPreviousSession()
*/
public function hasSession()
{
return null !== $this->session;
return null !== $this->session && $this->session->isStarted();
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. The hasSession() must return true when a request is associated with a session, that's all. The fact that the session is started or not is not the same thing.

Copy link

Choose a reason for hiding this comment

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

To clarify the meaning for discussions elsewhere: hasSession() means does this have a session object. Its not a reflection of the state of the session object.

Copy link

Choose a reason for hiding this comment

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

@fabpot - why did you merge this particular commit if you also disagree with the above diff?

Copy link
Member

Choose a reason for hiding this comment

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

because I reverted this part in another commit.

@fabpot
Copy link
Member
fabpot commented Jul 1, 2012

I'm +1 to add the isStarted() method, but -1 for the change of Request::hasSession.

@ghost
Copy link
ghost commented Jul 1, 2012

@fabpot F438 , I agree. hasSession() should not be changed, it's semantically incorrect to make it return effectively "hasActiveSession".

fabpot added a commit that referenced this pull request Jul 1, 2012
@fabpot
merged branch dlsniper/session-start-fix (PR #4541)
d2b5208 
Commits
-------

f72ba0a Fixed detection of an active session

Discussion
----------

[WIP][HttpFoundation][Session] Fixed detection of an active session

Bug fix: yes
Feature addition: no
Backwards compatibility break: not sure
Symfony2 tests pass: no
Fixes the following tickets: #4529
Todo: Fix failing tests
License of the code: MIT
Documentation PR: ~

This fixes the problem when the session variable inside $request now has always data in it as it's now more powerful but this introduces the problem that the old way of detecting if a session is started or not doesn't work anymore.

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

by travisbot at 2012-06-09T21:53:17Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1578839) (merged 9ae13e12 into 6266b72).

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

by drak at 2012-06-10T01:57:59Z

Sessions should be started implicitly. The SF auto_start config parameter controls the session listener to start the session.

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

by dlsniper at 2012-06-11T06:46:02Z

So this patch is correct then and I should continue the work on it?

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

by drak at 2012-06-11T07:51:39Z

@dlsniper - no it's not correct.  The session should not be auto-started like this, @fabpot and I recently discussed it.

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

by dlsniper at 2012-06-11T07:52:55Z

@Drak, ok I'll remove the patch for auto_start then but the fix for start would still stand, right?

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

by drak at 2012-06-12T18:40:35Z

@dlsniper - I have no objection to the rest of the PR except for the autostart stuff.  I've annotated for clarity :)

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

by travisbot at 2012-06-12T19:51:12Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1604158) (merged 3499980e into 37550d2).

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

by travisbot at 2012-06-12T19:52:00Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1604166) (merged dcc73071 into 37550d2).

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

by dlsniper at 2012-06-12T19:56:51Z

Seems Travis doesn't like the squashing of commits that I've did but the PR does pass the normal tests.
@Drak is this good for merging now?

Thanks :)

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

by dlsniper at 2012-06-13T09:05:09Z

@fabpot this can be merged safely, I've just applied the patch on my production application and the patch is ok, it's just travis failing.

Thanks

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

by travisbot at 2012-06-13T09:23:46Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1608735) (merged 1a6eabd2 into 37550d2).

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

by travisbot at 2012-06-13T09:28:26Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1608758) (merged 4e3a93c8 into 37550d2).

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

by dlsniper at 2012-06-13T09:29:28Z

I've noticed that this is failing, I'll fix it later on today.

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

by travisbot at 2012-06-13T15:14:01Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1611541) (merged 5504c4b7 into 37550d2).

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

by drak at 2012-06-13T15:23:47Z

It's possible that other tests are failing not related to this PR. Run the tests on the current master, and try rebasing your branch to the current master also.

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

by dlsniper at 2012-06-13T15:44:22Z

I've just reminded why this is failing on builds, I can't do them locally because of this:
```
Installing dev dependencies
Your requirements could not be solved to an installable set of packages.

        Problems:
                - Problem caused by:
                        - Installation request for doctrine/orm [>= 2.2.0.0, < 2.4.0.0-dev]: Satisfiable by [doctrine/orm-2.2.2, doctrine/orm-2.2.1, doctrine/orm-2.2.0, doctrine/orm-2.2.x-dev, doctrine/orm-2.3.x-dev].
```

I'll try and install this somehow and see what's wrong with it.

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

by mvrhov at 2012-06-13T18:08:58Z

@dlsniper: as @stof said to me this should be resolved in latest versions of composer, but it seems that is not. The problem is that composer cannot figure out that you are on dev-master if you try to instal dev. dependencies on feature branch. Take a look at the .travis.yml file on how to do a proper dev vendors install.
cc @Seldaek

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

by dlsniper at 2012-06-13T23:08:53Z

@mvrhov Thanks for pointing this out.

@Drak I still got two tests not passing but I'm not sure how to fix them as adding $session->start() will either fail with the message that the session has already been started, the headers_sent() call which returns true. Any help with them will be greatly appreciated. Thanks!

Here is what the HttpKernel tests are returning:
```
There were 2 failures:

1) Symfony\Component\HttpKernel\Tests\EventListener\LocaleListenerTest::testDefaultLocaleWithSession
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'es'
+'fr'

/var/www/symfony-orig/src/Symfony/Component/HttpKernel/Tests/EventListener/LocaleListenerTest.php:51

2) Symfony\Component\HttpKernel\Tests\EventListener\LocaleListenerTest::testLocaleFromRequestAttribute
Expectation failed for method name is equal to <string:set> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

FAILURES!
Tests: 263, Assertions: 1025, Failures: 2, Skipped: 10.
```

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

by travisbot at 2012-06-13T23:42:59Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1614883) (merged 1004b7c0 into c07e916).

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

by travisbot at 2012-06-13T23:53:06Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1614897) (merged f72ba0a into c07e916).

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

by dlsniper at 2012-06-16T20:14:41Z

@stof / @vicb Hi, do either of you think that you can either point me out to the right direction for fixing this either ping someone else for home help as @Drak doesn't seem available for this and at the moment I'm pretty much clueless in what direction I should take this fix.

Thanks!

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

by dlsniper at 2012-06-19T14:16:29Z

ping @fabpot Can you please provide some input on this one as I'm a bit stuck and seems noone else is available.

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

by drak at 2012-06-20T10:24:43Z

fyi - I'll be able to look again in a few days

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

by fabpot at 2012-07-01T07:53:28Z

I'm +1 to add the `isStarted()` method, but -1 for the change of `Request::hasSession`.

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

by drak at 2012-07-01T09:06:15Z

@fabpot, I agree. `hasSession()` should not be changed, it's semantically incorrect to make it return effectively "hasActiveSession".
@fabpot fabpot merged commit f72ba0a into symfony:master Jul 1, 2012
@dlsniper dlsniper mentioned this pull request Nov 18, 2012
Closed
@dlsniper
Copy link
Contributor Author

@fabpot Since you revered the change in the hasSession() method and there's no isStarted() method, how should we fix this issue?

I say again, since the refactoring @Drak did, the session object is now always present so if in 2.0.X the code was properly reporting if the user has a session active or not and now it always reports true, I'm not sure what needs to be node not to affect the function and keep the functionality as well.

Thanks!

fabpot added a commit that referenced this pull request Dec 1, 2012
@fabpot
[HttpFoundation] added a small comment about the meaning of Request::…
7b234db 
…hasSession() as this is a recurrent question (refs #4541)
fabpot added a commit that referenced this pull request Dec 3, 2012
@fabpot
Merge branch '2.1'
b22da6e 
* 2.1:
  [TwigBundle] Moved the registration of the app global to the environment
  needs to use simpleContent in xsd to allow empty elements
  bumped Symfony version to 2.1.5-DEV
  bumped Symfony version to 2.0.19-DEV
  removed wrong routing xsd statement `mixed="true"`
  removed unused attribute from routing.xsd
  [HttpFoundation] added a small comment about the meaning of Request::hasSession() as this is a recurrent question (refs #4541)
  updated VERSION for 2.1.4
  updated CHANGELOG for 2.1.4
  updated VERSION for 2.0.19
  update CONTRIBUTORS for 2.0.19
  updated CHANGELOG for 2.0.19

Conflicts:
	src/Symfony/Component/HttpKernel/Kernel.php
	src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd
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
2.1
Development

Successfully merging this pull request may close these issues.
5 participants
@dlsniper @travisbot @mvrhov @fabpot @stloyd
0