[2.1] Support PHP 5.4 \SessionHandler #3493
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks good to me. Can you update the CHANGELOG and UPGRADE file accordingly and start to update the documentation at symfony/symfony-docs? Thanks for your work, the session handling in Symfony2 is starting to become amazing! |
| } | ||
|
|
There was a problem hiding this comment.
you should remove this extra empty line
|
@fabpot I will start working on documentation this week and get the CHANGELOG/UPGRADING committed shortly. I'll ping when done. |
| */ | ||
| public function open($savePath, $sessionName) | ||
| { | ||
| $return = (bool)$this->handler->open($savePath, $sessionName); |
There was a problem hiding this comment.
missing space after the casting (same for all methods)
There was a problem hiding this comment.
fyi - check_cs doesn't pick it up. Time for some real php_codesniffer :)
There was a problem hiding this comment.
OpenSky has a phpcs config, but not sure it catches everything either
added 17 commits
March 14, 2012 20:15
…ation property. Revert service back to session.storage.native Rename session.storage.native_file to session.handler.native_file (which is the default so no BC break from 2.0)
It does not make sense to try and store session ini directives since they can be changes outside of the class as they are part of the global state. Coding stan
…nd stop polluting global namespace. This makes mock sessions truly mock and not to interfere with global namespace. Add getters and setters for session name and ID.
Native here refers to the fact the session storage interacts with real PHP sessions.
|
@fabpot - This PR is ready now. |
fabpot
added a commit
that referenced
this pull request
Mar 15, 2012
Commits ------- eb9bf05 [HttpFoundation] Remove hard coded assumptions and replace with API calls. 9a5fc65 [HttpFoundation] Add more tests. 68074a2 Changelog and upgrading changes. 7f33b33 Refactor SessionStorage to NativeSessionStorage. b12ece0 [HttpFoundation][FrameworkBundle] Separate out mock session storage and stop polluting global namespace. d687801 [HttpKernel] Mock must invoke constructor. 7b36d0c [DoctrineBridge][HttpFoundation] Refactored tests. 39526df [HttpFoundation] Refactor away options property. 21221f7 [FrameworkBundle] Make use of session API. cb873b2 [HttpFoundation] Add tests and some CS/docblocks. a6a9280 [DoctrineBridge] Refactor session storage to handler. a1c678e [FrameworkBundle] Add session.handler service and handler_id configuration property. 1308312 [HttpFoundation] Add and relocate tests. 88b1170 [HttpFoundation] Refactor tests. 2257a3d [HttpFoundation] Move session handler classes. 0a064d8 [HttpFoundation] Refactor session handlers. 2326707 [HttpFoundation] Split session handler callbacks to separate object. bb30a44 [HttpFoundation] Prepare to split out session handler callback from session storage. Discussion ---------- [2.1] Support PHP 5.4 \SessionHandler Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - This patch allows us to add services, like an encryption layer into any session handler without having to alter or inherit any code across any session handler, internal or custom. The `\SessionHandler` class exposes internal PHP's native internal session save handlers like files, memcache, and sqlite by wrapping the internal callbacks through the class giving user-space the chance to intercept, override and filter them by inheriting from `\SessionHandler`. I've written a pretty nice use-case at http://docs.php.net/sessionhandler which really shows the power of it. I never considered how to make proper use of the `\SessionHandler` in Symfony2 until I wrote the code example you see in that documentation and also because of the `AbstractSessionStorage` base class got in the way. It's really trivial to enable support for this in Symfony2 but requires to separate out the actual handlers because inheritance is not suitable. Obviously, the feature will only work with internal PHP-extension provided handlers under PHP 5.4 and will already work in PHP 5.3 with any custom handler (since they all implement `\SessionHandlerInterface`). Symfony2 will also be the first framework to support these amazing features :-D The necessary changes are really small but beautiful: The basic idea is this: https://github.com/drak/symfony/commit/1d55d1ff148125762641a337acde7163d7936e90 removed inheritance and separates out the actual session handler callbacks - the part PHP processes internally. This is supported by an internal proxy mechanism: https://github.com/drak/symfony/commit/10a36c901e9ee77255954539b2dc3e8920189002 In terms of BC, not much changes net from 2.0: - We can restore the deprecated service ID: `session.storage.native` - We add a new service ID `session.handler` (and configuration alias `handler_id`) for the actual session handlers. This defaults to the renamed `session.handler.native_file` session handler (same behaviour just new name and as it's a default there is no BC break). --------------------------------------------------------------------------- by fabpot at 2012-03-03T12:15:10Z Looks good to me. Can you update the CHANGELOG and UPGRADE file accordingly and start to update the documentation at symfony/symfony-docs? Thanks for your work, the session handling in Symfony2 is starting to become amazing! --------------------------------------------------------------------------- by drak at 2012-03-04T11:09:31Z @fabpot I will start working on documentation this week and get the CHANGELOG/UPGRADING committed shortly. I'll ping when done. --------------------------------------------------------------------------- by drak at 2012-03-14T16:48:37Z @fabpot - This PR is ready now.
| public function setId($id) | ||
| { | ||
| if ($this->started) { | ||
| throw new \LogicException('Cannot set session ID after the session has started.'); |
There was a problem hiding this comment.
Hmm, this exception throws in test environment for any $client->request
There was a problem hiding this comment.
Do you have a specific example to show?
There was a problem hiding this comment.
Can you link to the specific tests it breaks please? I tried to look for them but didnt see anything.
There was a problem hiding this comment.
Hi,
I not find tests, but they start issue
FriendsOfSymfony/FOSFacebookBundle#79
but fix not work :)
2012/3/22 Drak <
reply@reply.github.com
@@ -79,11 +110,35 @@ public function regenerate($destroy = false)
*/
public function getId()
{
if (!$this->started) {return '';return $this->id;- }
- /**
\* {@inheritdoc}*/- public function setId($id)
- {
if ($this->started) {throw new \LogicException('Cannot set session ID after theCan you link to the specific tests it breaks please? I tried to look for
them but didnt see anything.
Reply to this email directly or view it on GitHub:
https://github.com/symfony/symfony/pull/3493/files#r588565
fabpot
added a commit
that referenced
this pull request
Mar 23, 2012
Commits ------- c4ee947 Native Redis Session Storage update 665f593 NativeRedisSessionStorage added Discussion ---------- [HttpFoundation] Native Redis Session Storage Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - --------------------------------------------------------------------------- by lstrojny at 2012-03-04T23:15:43Z Does Symfony (or any of its dependencies) has Redis support in any form whatsoever? If not this might be a good point to decide which clients to support --------------------------------------------------------------------------- by lsmith77 at 2012-03-04T23:36:11Z well ideally we just get this cache interface stuff done .. for this use case it would be perfect. --------------------------------------------------------------------------- by pulzarraider at 2012-03-05T00:35:59Z There is RedisProfilerStorage available (based on phpredis). I prefer and write code for [phpredis](https://github.com/nicolasff/phpredis). It's recommended by [official Redis homepage](http://redis.io/clients#PHP). [In this benchmark](http://dev.af83.com/2011/01/01/which-php-library-to-use-with-redis-the-benchmark.html ) is fastest and less memory consumpting. But if somebody prefer predis (with phpiredis), rediska or something other widely used, there are no limitations to add support of it to Symfony. My opinion is, that the C extension should be supported at first, because of good performance and native session storage support. Redis is quite young and the process of creating PHP clients is comparable to Memcache. There were created pure PHP Memcache clients in the past (Google found for example [this](http://www.phpclasses.org/browse/file/20284.html) and [this](http://code.blitzaffe.com/pages/phpclasses/files/memcached_client_52-12)), but they are not being used now. Everyone, who is seriously thinking about performance, is using only the C Redis/Memcache(d)/... extensions. --------------------------------------------------------------------------- by drak at 2012-03-05T07:40:06Z +1 on this PR. Needs a test written though. I don't think there is any need to wait for #3493 imo. I'll deal with it if this is merged before #3493. Are there any PHP ini settings for this for this driver or is everything via the `session.save_path` directive? (A quick look at the C code seems to indicate there are no explicit ini directives). --------------------------------------------------------------------------- by lstrojny at 2012-03-05T12:14:34Z @pulzarraider I don’t necessarily disagree with the usage of phpredis, I just wanted to bring up the issues of various clients and people having different preferences about them. --------------------------------------------------------------------------- by fabpot at 2012-03-05T14:46:22Z @pulzarraider Can you add some unit tests before I merge? --------------------------------------------------------------------------- by pulzarraider at 2012-03-11T20:19:57Z @Drak No there are no php.ini settings. Only RedisArray has some, but it's another feature. @fabpot I've added simple test based on other session storage tests. I planned to create a RedisSessionStorage, too, but I have no time for it now. This can be added later in another PR as it's independent from NativeRedisSessionStorage. --------------------------------------------------------------------------- by drak at 2012-03-12T02:21:25Z The code looks OK to me. --------------------------------------------------------------------------- by fabpot at 2012-03-15T06:05:27Z #3493 has been merged now. --------------------------------------------------------------------------- by pulzarraider at 2012-03-16T23:21:27Z Code updated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This patch allows us to add services, like an encryption layer into any session handler without having to alter or inherit any code across any session handler, internal or custom.
The
\SessionHandlerclass exposes internal PHP's native internal session save handlers like files, memcache, and sqlite by wrapping the internal callbacks through the class giving user-space the chance to intercept, override and filter them by inheriting from\SessionHandler. I've written a pretty nice use-case at http://docs.php.net/sessionhandler which really shows the power of it. I never considered how to make proper use of the\SessionHandlerin Symfony2 until I wrote the code example you see in that documentation and also because of theAbstractSessionStoragebase class got in the way.It's really trivial to enable support for this in Symfony2 but requires to separate out the actual handlers because inheritance is not suitable.
Obviously, the feature will only work with internal PHP-extension provided handlers under PHP 5.4 and will already work in PHP 5.3 with any custom handler (since they all implement
\SessionHandlerInterface). Symfony2 will also be the first framework to support these amazing features :-DThe necessary changes are really small but beautiful:
The basic idea is this: https://github.com/drak/symfony/commit/1d55d1ff148125762641a337acde7163d7936e90 removed inheritance and separates out the actual session handler callbacks - the part PHP processes internally.
This is supported by an internal proxy mechanism: https://github.com/drak/symfony/commit/10a36c901e9ee77255954539b2dc3e8920189002
In terms of BC, not much changes net from 2.0:
session.storage.nativesession.handler(and configuration aliashandler_id) for the actual session handlers. This defaults to the renamedsession.handler.native_filesession handler (same behaviour just new name and as it's a default there is no BC break).