Bug Report #3242

ORM __wakeup() needs to clear _db

Added by Lorenzo Pisani over 3 years ago. Updated over 3 years ago.

Status:ClosedStart date:09/13/2010
Priority:NormalDue date:
Assignee:Isaiah DeRose-Wilson% Done:

0%

Category:-
Target version:Kohana v3.x - v3.1.0
Resolution:fixed Points:

Description

this is related to #3181 but is an issue because when _db is an object in the session, it will have the _config 'cleaned' so will be unable to connect to the DB, this causes a huge bug in applications where you need to clear the session to keep going (caused because of the user object being in the session).

Isaiah suggests implementing the serializable interface which is nicer but I am going to implement a quick fix for now.

Please hold for pull request =D pull request will be for 3.1.x but should be ported back to 3.0.x

warning: this is an issue with any object you decide to put into the session that holds a database object because __sleep() is NOT reliable based on http://bugs.php.net/bug.php?id=52604


Related issues

Duplicated by Kohana v3.x - Feature Request #3181: __wakeup() improvements Closed 08/14/2010

Associated revisions

Revision bbb4b277
Added by Isaiah DeRose-Wilson over 3 years ago

Use the serializable interface instead of the magic _sleep/_wakup methods to avoid a bug in php that causes __sleep() to serialize the whole object when there is an exception. Fixes #3242

History

#1 Updated by Lorenzo Pisani over 3 years ago

here is the pull request: http://github.com/kohana/orm/pull/2 sorry but my patch is actually an API change because I needed to create a _db_group property to remove some functionality from the _db property.

#2 Updated by Lorenzo Pisani over 3 years ago

I am also uncertain that implementing the serializable interface will actually fix the issue (not sure if the php bug is there too) but it's worth a shot.

I also think the database object should be able to serialize and unserialize properly without causing this issue (with username and password being wiped)

#3 Updated by Isaiah DeRose-Wilson over 3 years ago

Making the ORM object implement serializable seems to fix this issue. You can see my commit for it below, I'm not sure if removing the _sleep()/_wakeup methods would be considers an api break, but if it isn't we should be able to merge this into 3.0.x as well.

http://github.com/isaiahdw/kohana-orm/commit/bbb4b27713e4b563ca316163ac70d346a3b1981a

#4 Updated by Isaiah DeRose-Wilson over 3 years ago

  • Assignee changed from John Heathco to Isaiah DeRose-Wilson

#5 Updated by Woody Gilk over 3 years ago

  • Target version changed from v3.0.8 to v3.0.9

#6 Updated by John Heathco over 3 years ago

Isaiah, I think you're good to go to fix this as planned for 3.0.9. Removing a magic method could still be considered an API break, but I see this being extremely minor and worth the fix... thoughts?

#7 Updated by Lorenzo Pisani over 3 years ago

Unfortunately since your current __sleep() method is a whitelist instead of a blacklist, I feel this is a big API change. Anyone storing extra data in the orm objects that get serialized will need to refactor their models to now store the extra data using the interface.

I still think the list of stored properties should be a blacklist for this reason and not a whitelist. When I add properties to my model that do not belong to the DB table, I should not need to add them to another method before they get serialized. We should simply remove the unrequired properties from the model when serializing. Zombor's AM does this by removing the database instance in __sleep() from the array of properties(I realize ORM would have a larger amount of blacklisted properties because it's a larger class).

Edit: here is how zombor does it http://github.com/zombor/Auto-Modeler/blob/master/classes/automodeler.php#L96

we could simply use a $_serialize_blacklist property with array('_db', '_table_name') etc

#8 Updated by Isaiah DeRose-Wilson over 3 years ago

Lorenzo Pisani wrote:

Unfortunately since your current __sleep() method is a whitelist instead of a blacklist, I feel this is a big API change.

Currently ORM uses a whitelist instead of a blacklist in the __sleep() method, so I did the same thing in the serialize method. I don't really see how this is an api change (maybe I'm missing something?).

#9 Updated by Lorenzo Pisani over 3 years ago

class Model_User extends ORM {
  public function __sleep()
  {
    $array = parent::__sleep();
    return $array + array('_cart');
  }
}

upgrading my app to 3.0.9 will now break the site because _cart no longer gets serialized

also, I am OK with your fix being a whitelist, I guess what I am saying is that I should make another ticket to turn it into a blacklist =P I was just mentioning the issue and how it's related to the fact that it's a whitelist.

#10 Updated by Isaiah DeRose-Wilson over 3 years ago

Lorenzo Pisani wrote:

upgrading my app to 3.0.9 will now break the site because _cart no longer gets serialized

Hmmm, I see your point now. Maybe it would be better to make them update their models though, instead of having dead __sleep() methods in them? I'm fine with a blacklist if John agrees.

#11 Updated by Isaiah DeRose-Wilson over 3 years ago

  • Project changed from Kohana v3.x to ORM
  • Category deleted (Modules:ORM)

#12 Updated by Isaiah DeRose-Wilson over 3 years ago

  • Status changed from New to Closed
  • Priority changed from Urgent to Normal
  • Target version changed from v3.0.9 to v3.1.0
  • Resolution set to fixed

I'm moving this to 3.1.x because it's really an api change. Fixed by 8b44bcae...

Also available in: Atom PDF