Error when retrieving and unserialising null DateTime fields

[matteorebeschi]

Hi,
I am using this functionality and I have encountered a bug when I try to retrieve a version of an entry that had a timestamp field with a NULL value.

IE, I have this Model:

<?php

namespace App\Model\Table;

use Cake\ORM\Table;
use Cake\Validation\Validator;

/**
 * News Model
 */
class NewsTable extends Table
{
    /**
     * Initialize method
     *
     * @param array $config The configuration for the Table.
     * @return void
     */
    public function initialize(array $config): void
    {
        parent::initialize($config);

        $this->setTable('news');
        $this->setPrimaryKey('id');

        $this->addBehavior(
            'Version',
            [
                'versionTable' => 'news_versions',
            ]
        );
    }

    /**
     * Default validation rules.
     *
     * @param Validator $validator
     * @return Validator
     */
    public function validationDefault(Validator $validator): Validator
    {
        $validator
            ->integer('id')
            ->allowEmptyString('id', 'create');

        $validator
            ->scalar('title')
            ->requirePresence('title', 'create')
            ->notEmptyString('title')
            ->maxLength('title', 200);

        $validator
            ->scalar('subtitle')
            ->allowEmptyString('subtitle')
            ->maxLength('subtitle', 300);

        $validator
            ->dateTime('visible_from')
            ->allowEmptyString('visible_from');

        $validator
            ->dateTime('visible_to')
            ->allowEmptyString('visible_to');
    }

<?php

namespace App\Model\Entity;

use Cake\ORM\Entity;
use Josegonzalez\Version\Model\Behavior\Version\VersionTrait;

/**
 * News Entity
 *
 * @property int $id
 * @property string $title
 * @property string $subtitle
 * @property \Cake\I18n\Time $visible_from
 * @property \Cake\I18n\Time $visible_to
 *
 * @property \App\Model\Entity\Version $version
 *
 * @property array $sitemap
 */
class News extends Entity implements SearchEngineInterface
{
    use VersionTrait;

    protected $_accessible = [
        'id' => false
    ];
}

When I save it, entries in the news_versions table are correctly created for each field, Since the visible_from and visible_to fields are set as nullable on my DB, they are correctly saved with a value of "N;".
When I retrieve the versions for my News entity by calling $news->versions(), I get this error:

2022-03-31 15:14:41 Error: [Exception] DateTimeImmutable::__construct(): Failed to parse time string (N;) at position 1 (;): Unexpected character in /var/www/repo/public/vendor/cakephp/chronos/src/Chronos.php on line 109

I have found out that this caused by the convertFieldsToType method, called in the groupVersions method of VersionBehavior.
This happens only for Datetime fields, (other nullable fields that are saved as "N;" are being unserialized correctly) and only after updating to CakePHP 4, with version 4.0.1 of this package. This was working fine with CakePHP 3.x, and version 2 of this package.
I have found that commenting the call to convertFieldsToType solves the problem (and in fact, this call wasn't present in version 2 of the library), but I'm not sure that that's the most correct way to go, therefore I haven't submitted a pull request.

Thanks.

[jeremyharris]

I think what needs to happen is that convertFieldsToType needs to inspect the column nullability in the $column var and use null if the value is null and it's a nullable column, instead of using the Type class to convert it.

If you can't get to a PR, let me know and I'll try to take care of it :)

[matteorebeschi]

Hello @jeremyharris ,
I've tried to change the code to achieve what you mentioned, but I noticed that, while doing that, I introduced (or discovered) another problem for the non-null Datetime fields.
In my DB they are being saved like this in the versions table:

O:14:"Cake\I18n\Time":3:{s:4:"date";s:26:"2021-06-14 16:38:34.099184";s:13:"timezone_type";i:3;s:8:"timezone";s:3:"UTC";}

which was leading to this error:

2022-04-04 10:45:43 Error: [Exception] DateTimeImmutable::__construct() [<a href='https://secure.php.net/datetimeimmutable.construct'>datetimeimmutable.construct</a>]: Failed to parse time string (O:14:&quot;Cake\I18n\Time&quot;:3:{s:4:&quot;date&quot;;s:26:&quot;2021-06-14 16:38:34.099184&quot;;s:13:&quot;timezone_type&quot;;i:3;s:8:&quot;timezone&quot;;s:3:&quot;UTC&quot;;}) at position 1 (:): Unexpected character in /var/www/repo/public/vendor/cakephp/chronos/src/Chronos.php on line 109

So, I had to change the behavior for those as well.

This is the final version I've arrived to for the convertFieldsToType function, which seems to be working for all kinds of fields.

    protected function convertFieldsToType(array $fields, $direction)
    {
        if (!in_array($direction, ['toPHP', 'toDatabase'])) {
            throw new InvalidArgumentException(sprintf('Cannot convert type, Cake\Database\Type::%s does not exist', $direction));
        }

        $driver = $this->_table->getConnection()->getDriver();
        foreach ($fields as $field => $content) {
            $column = $this->_table->getSchema()->getColumn($field);

            if ($column['null'] && is_null(unserialize($content))) {
                $value = null;
            } else {
                if ($column['type'] == 'datetime') {
                    $value = unserialize($content);
                } else {
                    $type = Type::build($column['type']);
                    $value = $type->{$direction}(unserialize($content), $driver);
                }
            }

            $fields[$field] = $value;
        }

        return $fields;
    }

I'm not sure if I should submit that as a pull request as I'm not a fan of all those IF statements.
Also, as I mentioned earlier, I found out that simply removing the call to the convertFieldsToType in the groupVersions method seem to be working just fine for me, so for the time being the solution I've found is extending the VersionBehavior in my project, with the only change being that that line is removed in the groupVersions method.

[jeremyharris]

It sounds like we need some tests against DateTime types. If you're comfortable with starting a PR with a failing test for a DateTime column, that might be the way to go. I don't recall if I've personally versioned a DateTime column and I don't see any tests for them, so they might not be supported yet.

If you comment calling convertFieldsToType it works because you'll just get the raw version data, but then none of your values come out in the type they went in as.

[matteorebeschi]

Hello @jeremyharris ,
I have opened a pull request with the failing test.