cphalcon
cphalcon copied to clipboard
[BUG]: getSnapshotData returns null
model getSnapshotData under some situations returns null , should be array otherwise calls getChangedFIelds throws "The 'keepSnapshots' option must be enabled to track changes"
the way serialize and unserialize work in Model.zep seem to be the problem
The first time i serialize a model , the attributes and snapshot seem to be identical, leaving the snapshot variable serialised in a null state
Follow up with an unserialize, then snapshot property is null, breaking calls to getChangedFIelds / hasChanged etc
i believe the default state of snapshot should not be null in serialize function as below.
public function serialize() -> string
{
/**
* Use the standard serialize function to serialize the array data
*/
var attributes, manager, dirtyState, snapshot = null;
let attributes = this->toArray(),
dirtyState = this->dirtyState,
manager = <ManagerInterface> this->getModelsManager(),
snapshot = [];
if manager->isKeepingSnapshots(this) && this->snapshot != null && attributes != this->snapshot {
let snapshot = this->snapshot;
}
return serialize(
[
"attributes": attributes,
"snapshot": snapshot,
"dirtyState": dirtyState
]
);
}
Details
- Phalcon version: 5.0.0alpha7
- PHP Version: 8.0.14
- Operating System: Ubuntu 20.04
- Installation type: Compiled from source
- Zephir version (if any):
- Server: Nginx
@Nialld I checked this and you are correct, the exception is thrown
Using your example (the below is a test):
- Insert a record in the DB
- Get the record from the db with a
keepSnapshots
on - Serialize it
- Unserialize it
- Call
getChangedFields()
Since the model has not changed, there is no snapshot data so the code that checks for that will throw the exception.
However, looking at it, it should not check the snapshot
property but check the models manager if the snapshots have been enabled. At that point you will not get an exception but you will end up with an empty array.
There was an error in the code which I fixed in my branch. The snapshot
property is always an array and never null
. It initializes as an empty array.
Can you have a look at this test:
https://github.com/niden/cphalcon/blob/T15837-snapshot/tests/database/Mvc/Model/GetSnapshotDataCest.php#L51
and let me know if I understand correctly the issue?
this issue is happening with caching involved
i have some steps to reproduce
$somemodel = SomeModel::findFirst(1); // SomeModel descends from \Phalcon\Mvc\Model .. reads the data from the DB
/** in the onConstruct of SomeModel public function onConstruct() { $this->getModelsManager()->keepSnapshots($this, $true); } **/
$cache = Di::getDefault()->getShared('cache'); /* @var $cache \Phalcon\Cache\Adapter\Libmemcached */ $cache->set('key', $somemodel, 100); $newModel = $cache->get('key'); $changedFields = $newModel->getChangedFields(); // BOOM
should also say, i was able to work around this by overriding unserialize in my class that extends \Phalcon\Mvc\Model
/**
* could be fixed by https://github.com/phalcon/cphalcon/issues/15837
* @param $data
* @return void
*/
public function unserialize($data)
{
parent::unserialize($data);
if (is_null($this->getSnapshotData())) {
$this->setSnapshotData([]); // fix the snapshot data
}
}
should also say, i was able to work around this by overriding unserialize in my class that extends \Phalcon\Mvc\Model
/** * could be fixed by https://github.com/phalcon/cphalcon/issues/15837 * @param $data * @return void */ public function unserialize($data) { parent::unserialize($data); if (is_null($this->getSnapshotData())) { $this->setSnapshotData([]); // fix the snapshot data } }
@Nialld In my branch this will not happen since I removed the null
from snapshotData
it is an empty array if it is empty.
I will run your example in a test and see what I get. Stay tuned.
this issue still exists, ive moved the fix into a custom serialiser as such
class CustomPhp extends \Phalcon\Storage\Serializer\Php
{
public function unserialize($data): void
{
parent::unserialize($data);
if ($this->data instanceof Model) {
if (is_null($this->data->getSnapshotData())) {
$this->data->setSnapshotData([]);
}
}
}
}
and when initializing my cache i add this class using setDefaultSerializer, probably not the best way but it works for me
A similar problem. Will it be fixed, @niden?
My observations:
The default property of the snapshot model is []
.
During serialize, if there were no changes to the model (for example, they just got it from the database) snapshot = null
and when this property is taken from the cache, null
is assigned to it ($model->getSnapshotData() === null
) provided that $this->ModelManager->isKeepingSnapshots($model) === true
.
During unserialize, this null
is written to the model. What prevents you from using $this->getChangedFields();
, where the snapshot property is checked for an array https://github.com/phalcon/cphalcon/blob/master/phalcon/Mvc/Model.zep#L1823
We get the error "The 'keepSnapshots' option must be enabled to track changes".
The cache is stored in radis, I work through symfony/cache
.
To Reproduce
class A extends Model
{
public $prop;
public function initialize()
{
$this->keepSnapshots(true);
}
}
// fail
$a = A::findFirst(1);
$this->getCache()->set('test', $a);
$cachedModel = $this->getCache()->get('test');
$cachedModel->getSnapshotData(); // null here
$this->getChangedFields(); // get error
Details
- Phalcon version: 5.1.4
- PHP Version: PHP 8.1.15
- Operating System: linux
- Installation type: pecl
- Server: Nginx
__serialization
sets snapshot = null
in the cache model if attributes are not changed.
https://github.com/phalcon/cphalcon/blob/5.0.x/phalcon/Mvc/Model.zep#L384
If the snapshot is cached, then __unserialize
set snapshot = cachedSnapshot
. And this condition does not work
https://github.com/phalcon/cphalcon/blob/5.0.x/phalcon/Mvc/Model.zep#L603
temporary fix in the model:
public function __serialize(): array
{
$arr = parent::__serialize();
if ($arr['snapshot'] === null) {
unset($arr['snapshot']);
}
return $arr;
}