Add __serialize/__unserialize #5
Conversation
|
Yay, failing test |
ChristophWurst
force-pushed
the
fix/magic-serialize-unserialize
branch
3 times, most recently
from
2b983ac to
7bb8c4e
Compare
| public function __serialize() | ||
| { | ||
| return array( | ||
| 'rights' => $this->_rights |
There was a problem hiding this comment.
strictly speaking this is not compatible with the old serialization methods but https://www.php.net/manual/en/language.oop5.magic.php#object.serialize has to return an array and $this->_rights is false|array.
| /** | ||
| * @return array | ||
| */ | ||
| public function __serialize() |
There was a problem hiding this comment.
same here. the old one serialized to a string, now need to use an array of string
Codecov Report
@@ Coverage Diff @@
## master #5 +/- ##
============================================
- Coverage 32.34% 32.01% -0.33%
- Complexity 3256 3286 +30
============================================
Files 80 80
Lines 7721 7777 +56
============================================
- Hits 2497 2490 -7
- Misses 5224 5287 +63
Continue to review full report at Codecov.
|
|
I think this is OK now. @bytestream please have another look :) |
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
force-pushed
the
fix/magic-serialize-unserialize
branch
from
9d09302 to
b94b163
Compare
|
Squashed |
| $data = @unserialize($data); | ||
| if (!is_array($data) || | ||
| !isset($data['v']) || | ||
| $this->__unserialize(@unserialize($data)); |
There was a problem hiding this comment.
I'm slightly uneasy about @ and lack of is_array(), because it will now throw a Throwable instead of an Exception('Cache version changed'). It's unclear to me why @ is used as it's not used in all classes, and also unclear whether the Exception is caught somewhere... but catch (\Exception) is pretty bad anyway so 🤷♂️
| } | ||
|
|
||
| /** | ||
| */ | ||
| public function unserialize($data) | ||
| { | ||
| list($this->_required, $this->_optional) = json_decode($data); | ||
| $this->__unserialize(json_decode($data, true)); |
There was a problem hiding this comment.
I've also added the true param here. This was obviously broken. json_decode would have deserialized into an object and list expects an array …
|
LGTM 👍 |
This was more work than I want to admit. Yet this is fully untested.
Upstream PR: horde#21