fix(media): restore Media state on unserialize from Doctrine cache#88
fix(media): restore Media state on unserialize from Doctrine cache#88BriceFab wants to merge 3 commits intojolicode:mainfrom
Conversation
BriceFab
changed the title
|
@BriceFab Thx a lot, could you run |
Media entities cached by Doctrine (e.g. second level cache) are recreated via __wakeup/__unserialize without running the constructor. Restore path, storage, and related state from serialized payload and resolver. - Implement __serialize/__unserialize with backward-compatible array shape - Register __unserialize in JoliMediaBundle for Symfony 7.1+ native lazy ghost compatibility - Add regression test for unserialization round-trip Made-with: Cursor
BriceFab
force-pushed
the
fix/media-unserialize-slc-87
branch
from
925e45a to
0cc9424
Compare
|
@loic425 done |
| $resolvedMedia = $this->getResolver()->resolveMedia($path, $libraryName); | ||
| } catch (MediaNotFoundException) { | ||
| $resolvedMedia = $this->getResolver()->createUnresolvedMedia($path, $libraryName); | ||
| } |
There was a problem hiding this comment.
@xavierlacot Any opinion about this resolver embedded in the DTO? 🤔 I'm not sure we need to force those resolution when unserializing. Having the resolver attached to the DTO feels weird.
There was a problem hiding this comment.
When we store something with a state, there is no reason to retrieve the same object from cache with a different state. It should be the same state, the same object.
Introducing a resolver here leads to unexpected output from the cache.
For example, when you get your doctrine entity from the cache, there is no refresh with the original entity from the table, you get what you stored.
There was a problem hiding this comment.
I think we could serialize the storage into the __serialize method and then we will be able to deserialize the media without the resolver.
if (!isset($data['storage']) {
throw Invalid...
}
$this->path = $data['path'];
$this->storage = $data['storage'];
| return [$this->path, $this->storage->getLibrary()->getName()]; | ||
| return [ | ||
| 'path' => $this->path, | ||
| 'library' => $this->storage->getLibrary()->getName(), |
There was a problem hiding this comment.
that'd be cool to serialize the storage to be able to deserialize the media without the resolver.
| 'library' => $this->storage->getLibrary()->getName(), | |
| 'storage' => $this->storage->__serialize(), |
There was a problem hiding this comment.
Hum the serialize method on the storage seems not ok...
3 participants
Summary
Media::__unserialize()so cachedMediainstances restore typed properties correctly[0 => path, 1 => library])Media::$resolverInitializerin bundle boot (same resolver used by Doctrine media types)Problem
When an entity containing
JoliCode\MediaBundle\Model\Mediais read from Doctrine second-level cache, unserialization may create dynamic properties ($0,$1) instead of restoring constructor-backed typed properties.This can leave
$pathuninitialized and later crash at runtime whengetPath()is accessed.Validation
tests/src/Model/MediaTest.phpfor:Fixes #87
cc @xavierlacot