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

Serializing a Collection results in error when unserializing and manipulating it #6228

Closed
ramiroaraujo opened this issue Mar 31, 2015 · 20 comments
Milestone

Comments

@ramiroaraujo
Copy link
Contributor

I found this while caching some database results, that ended up wrapped in a Collection. When serializing the collection compile returns, the unserialized Collection would return PHP Fatal error: Uncaught exception 'LogicException' with message 'The object is in an invalid state as the parent constructor was not called' in ... when trying to perform any operation on it.

If the case is that any collection will fail if reconstructed from a serialization, then we should document that and throw an Exception when trying to serialize, but I'm not sure that's the case, and I'm not familiar with the internals of Colleciton and it's friends.

Happy to implement and document this, but would like to be sure first :)

@lorenzo
Copy link
Member

lorenzo commented Mar 31, 2015

What code triggered this error?

@lorenzo lorenzo added this to the 3.0.1 milestone Mar 31, 2015
@lorenzo lorenzo added the defect label Mar 31, 2015
@lorenzo
Copy link
Member

lorenzo commented Mar 31, 2015

I think implementing serialize() in the collection class may be the best option

@ramiroaraujo
Copy link
Contributor Author

$collection = new Cake\Collection\Collection([1,2,3,4,5]);
$invalid = unserialize(serialize($collection));
$invalid->toArray();
// -> PHP Fatal error:  Uncaught exception 'LogicException' with message 'The object is in an invalid state as the parent constructor was not called' in ...

@ramiroaraujo
Copy link
Contributor Author

would you say there's not anything inherently incompatible with serializing a Collection? I'm not too keen on this, but for example I understand there's currently no way of serializing a closure. Most collections I use have a bunch of them :)

@lorenzo
Copy link
Member

lorenzo commented Mar 31, 2015

The main problem of serializing collections is deciding whether you want to serialize with array keys or without them, but in general all collections can be serialized if they are rewindable. I can guide you to implement this feature if you like

@ramiroaraujo
Copy link
Contributor Author

sure, wouldn't know where to start though :)
btw, collections are implemented in a decorator fashion?

@lorenzo
Copy link
Member

lorenzo commented Mar 31, 2015

Yes, the main pattern used to implement collections is Decorator.

Start by adding and implementing this interface in the Collection object http://php.net/manual/en/class.serializable.php

@ramiroaraujo
Copy link
Contributor Author

that's the easy part :)
I'll start and try to advance as much as possible. I'll keep this ticket posted

@markstory markstory added the ORM label Mar 31, 2015
@markstory
Copy link
Member

@lorenzo Couldn't you also process the collection into a static set of data and serialize the results of that? Similar to serializing the results of toArray()?

@lorenzo
Copy link
Member

lorenzo commented Mar 31, 2015

That is the idea that I have in mind, but using to array will not be enough. It needs to eb an object having items stored in this way:

[
  [key1, value1],
  [key2, value2],
  [key3, value3],
  ...
]

So when unserialized, the person can choose whether to use toArray, toList or simple iterate over it. This is because keys can be repeated in collections, and also because keys may be objects.

@ramiroaraujo
Copy link
Contributor Author

@markstory what do you mean by that?
I've implemented the Serielizable interface but I dont even have a clue on where to begin.

Here's my (little) understanding of the whole Collection impl + SPL iterators: A Collection begins with an initial Array or Traversable object. Then when you start manipulating the collection each method wraps the previous Iterator adding each manipulation. Nothing is executed until iterated or the toArray or compile method is called. When called, and this is the part I understand the less, the _unwrap method does something I cant fully understand, but what usually happens is that each item in the Traversable object starts going up the layers of transformations (some get filtered in the way) until a final Collection is returned or some computed value. Ir order to to this, usually Closures are involved, and since most Collection methods accept callables, class or instance methods could be involved, or global functions, etc. Now, how on earth do we serialize that? :|
Or what are you thinking when we talk about serializing a Collection?
@lorenzo don't even understand your last comment! jahahha

sorry, I'm trying to help, but also trying to learn and deeply understand this

@lorenzo
Copy link
Member

lorenzo commented Mar 31, 2015

Hehe, don't worry. So to make the task more specific, this is the outline of what needs to be done:

  • Create a new MultiKeystIterator class in Collection/Iterator:

This class implements Collection and uses the CollectionTrait and implements the method add($key, $value). This method will store the values in an internal array of the form:
[ [$key, $value], [$key, $value]]

So for each call to add() you get a new array with 2 positions.

  • Implement all Iterator methods in MultiKeystIterator.

In particular, for each element the method key() should return the current element $array[0] and current() should return the current element array[1]

  • Implement MultiKeystIterator::serialize()

It will take the internal array and serialize it: return serialize($this->getInnerIterator())

  • Implement Collection::serialize(). The pseudo-code for it is:
$multi = new MultiKeysIterator();
foreach ($this as $key => $element) {
    $multi->add($key, $element);
}
return serialize($multi);
  • Implement Collection::unserialize(). The pseudo-code for it is:
$unserialized = unserialize($serializedString);
return new Collection(new MultiKeysIterator($unserialized));

@ramiroaraujo
Copy link
Contributor Author

heh, you sound very confident! perfect, I'll do it

@ramiroaraujo
Copy link
Contributor Author

can you help me understand the reasoning behind all this? :)

@markstory
Copy link
Member

Simply calling toArray() won't work as the unserialized result could have toList() called on it, and the results would not match the pre-serialized object. Also because of how iterators work, keys can be duplicated which is why @lorenzo is recommending the data be stored as [$key, $value]. This structure also allows you to preserve the ability to use toArray() and toList() on the unserialized object.

@lorenzo
Copy link
Member

lorenzo commented Mar 31, 2015

@ramiroaraujo Actually, I just remember where this implementation idea came from... It is already implemented!! BufferedIterator``is actuallyMultiKeysIterator`

So, no need to implement that again. We just need to implement BufferedIterator::serialize() and Collection::serialize()

@ramiroaraujo
Copy link
Contributor Author

great! :D
I'll check that for clarifications first

@ramiroaraujo
Copy link
Contributor Author

@lorenzo I've been trying out, but hit a few dead ends.

  1. the BufferedIterator doesn't look quite like you described it should. There's a key and current property, but no add method.
  2. The constructor creates an instance of SplDoublyLinkedList, which has and add method that kind of look similar, but just initializes it empty
  3. Trying to call SplDoublyLinkedList::add results in an Call to undefined method error :|
  4. The only method in BufferedIterator that seems to do something is BufferedIterator::valid, but I don't seem to get what's it's doing or if it's relevant to our cause.

as an extra, the Serializable interface dictates that the unserialize method should not return anything... woooooooow?

@lorenzo
Copy link
Member

lorenzo commented Apr 1, 2015

would it be easier for you to implement MultiKeysIterator yourself?

@markstory markstory modified the milestones: 3.0.1, 3.0.2 Apr 5, 2015
@markstory markstory modified the milestones: 3.0.2, 3.0.3 Apr 20, 2015
@lorenzo
Copy link
Member

lorenzo commented Apr 25, 2015

I'm closing this since the way of caching database results is working correctly. We have decided to not implement the Serializable interface for collections as it makes the code much more difficult to sue and understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants