[HttpFoundation] Serialization error with PdoSessionStorage #3000
Comments
|
What would you suggest doing here? Make an effort to serialize it or throw an error? I guess the reason PDO is not serializable is that it would require access to the DB connection details which should not be available for display. Without those, you cant really unserialize and so an |
|
Maybe it's better to delegate the serialization to the storage object. That way the PdoSessionStorage class can provide the serialized data without the connection details. |
|
I'm not sure the problem is where there serialization happens, it's that you cannot serialize |
|
The example on fabpot's blog is ok for the case of exceptions, but the forward action surely needs to unserialize the session object so just blotting out the |
|
Any new thoughts on this yet? |
|
So basically, we can add the |
|
is it needed to forward the request ? The place where the exception occured here is the DataCollector. In this place, we only need to be able to display the content of the session in the profiler. We don't need to be able to access the database again (as the database would have changed anyway). So serializing the session does not need to serialize the storage for the profiler, but only its content. @fabpot do we serialize the session in another place ? |
|
If the unserialisation doesn't matter then there are two approached we can take. Either, simply add the Remember this issue exists already since Symfony 2.0 /cc @fabpot |
Commits ------- 2c767d1 [HttpFoundation] Fixed closeSession for the Memcached storage ec44e68 [HttpFoundation] Fixed the use of the prefix for the Memcached storage 5808773 [HttpFoundation] Fixed a typo and updated the phpdoc for the session 0550bef Removed methods duplicated from parent class Discussion ---------- Session cleanup This cleans the phpdoc of the refactored session by using inheritdoc in all appropriate places. For the SessionInterface, I added the ``@api`` tag in all methods as the Session class had them. It also fixes a few typos in the variable names. I figured a few things: - the Session class implements some methods that are not part of the SessionInterface. Is it intended or simply a left-over ? - the MemcachedSessionStorage uses a ``prefix`` property which does not exist. This is clearly a copy-paste from the MemcacheSessionStorage which has it. It also call the ``Memcached#close`` method which does not exist according to PhpStorm. @Drak could you check this class ? I don't know Memcached at all. - as I said on the refactoring PR, the Serializable implementation of the Session class seems totally wrong as the SessionStorage is not serializable. /cc @Drak --------------------------------------------------------------------------- by drak at 2012-02-12T02:09:38Z @stof there is a ticket about this, the problem exists from Symfony 2.0 also - refs #3000 PDOSessionStorage --------------------------------------------------------------------------- by drak at 2012-02-12T02:10:12Z @stof I will look at the memcache issue and make a quick PR --------------------------------------------------------------------------- by stof at 2012-02-12T02:11:07Z @Drak the issue could exist with any SessionStorage as your SessionStorageInterface does not enforce making it serializable --------------------------------------------------------------------------- by stof at 2012-02-12T02:14:15Z @Drak note that this PR already fixes 2 typo in the memcached storage. It seems like some tests are missing for it as they should have been found (they would have raised a notice in the constructor) --------------------------------------------------------------------------- by drak at 2012-02-12T02:14:50Z Afaik, only PDO is not serializable@ but the `Serializable` is on the SessionInterface. The problem is with #3000, the serialization is necessary but impossible. --------------------------------------------------------------------------- by stof at 2012-02-12T02:21:42Z @Drak what about a Mongo storage or things like that ? --------------------------------------------------------------------------- by drak at 2012-02-12T02:23:58Z @stof Since you've started this PR would you mind removing `$prefix.` from the `*session()` methods in `MemcachedSessionStorage` - they are not required because Memcached handles prefixes itself. I'll write some tests in another PR for them. I guess I omitted it because at the time I didn't have them on my test env. --------------------------------------------------------------------------- by drak at 2012-02-12T02:24:35Z @stof - Let's take the serialization issue to #3000 because it's being discussed there. --------------------------------------------------------------------------- by stof at 2012-02-12T02:27:27Z @Drak what about the ``close()`` method which does not exist in the Memcached class ? --------------------------------------------------------------------------- by drak at 2012-02-12T03:58:11Z The method just needs to `return true;`.
|
I'm not able to reproduce the error on master and I don't see where the session is actually serialized (even in 2.0). Serializing the session is anyway a bad idea; what we need to do is serialize the data within the session but not the session itself. So, I've removed the \Serialize interface for the session for now and if it breaks something, I will happily fix it in the proper way. |
|
Closing this issue as a fix has been committed with no negative feedback on it. |
When using PdoSessionStorage, an exception is thrown by PHP when the session is serialized:
I think the Session class needs to check if the storage can be serialized, maybe by using a NotSerializable interface, as @fabpot blogged about a while ago.
FYI: this issue is caused by
Symfony\Bundle\FrameworkBundle\DataCollector\RequestDataCollector::serialize()and only occurs when I forward a request. I'm using the latest Symfony version to date (2.0.8).The text was updated successfully, but these errors were encountered: