Change or new Feature

[pine3ree]

I was wondering why the DateTimeFormatterStrategy allows the empty string during hydration:

laminas-hydrator/src/Strategy/DateTimeFormatterStrategy.php

Line 112 in cd865fc

if ($value === '' || $value === null || $value instanceof DateTimeInterface) {

Since we are hydrating a DateTime|null property, shouldn't we hydrate an empty datetime string with null?
Or we could add a constructor option to let the developer choose in those cases (private $hydrateEmptyStringToNull = false; to avoid BC)

kind regards

[froschdesign]

Please check Laminas\Hydrator\Strategy\NullableStrategy: https://docs.laminas.dev/laminas-hydrator/v4/strategy/#laminas-hydrator-strategy-nullablestrategy

[pine3ree]

Hello @froschdesign,

Please check Laminas\Hydrator\Strategy\NullableStrategy: https://docs.laminas.dev/laminas-hydrator/v4/strategy/#laminas-hydrator-strategy-nullablestrategy

Yes I am aware of that strategy. Still, it sounds a bit strange (at least to me) to let a string pass while hydrating a DateTimeInterface prop. In most cases the class setter would only accept ?DateTimeInterface.

kind regards

[pine3ree]

Another option would be to use it as the constructor argument for DateTime, it is still a valid argument and results in current datetime..

[froschdesign]

@pine3ree
An example says more:

class Album
{
    private ?DateTimeImmutable $releaseDate;

    public function getReleaseDate(): ?DateTimeImmutable
    {
        return $this->releaseDate;
    }
}

$hydrator = new Laminas\Hydrator\ReflectionHydrator();
$hydrator->addStrategy(
    'releaseDate',
    new Laminas\Hydrator\Strategy\NullableStrategy(
        new Laminas\Hydrator\Strategy\DateTimeImmutableFormatterStrategy(
            new Laminas\Hydrator\Strategy\DateTimeFormatterStrategy('Y-m-d')
        ),
        true
    )
);
$album = new Album();

$hydrator->hydrate(
    [
        'releaseDate' => '2021-02-05',
    ],
    $album
);
var_dump($album->getReleaseDate()->format('d.m.Y')); // "05.02.2021"

$hydrator->hydrate(
    [
        'releaseDate' => '',
    ],
    $album
);
var_dump($album->getReleaseDate()); // NULL

[pine3ree]

Yes of course @froschdesign , You didn't have to write the full example :-). But maybe I have been not very clear. My point is that a DateTime hydrator (or any other kind) should be doing that on it's own, without a wrapping nullable-strategy, and not hydrate with unrelated values. I was migrating a zf3 project in which I had written my own (DateTime|Date|Boolean|Float)Strategy classes and I was comparing the differences in the implementations.

To be short: an hydrator should only hydrate with consistent values. In this case an empty string is not interchangeable with a DateTime object. But this could apply to other typed-hydrations. My idea is to "only pass acceptable/consistent hydrating values".

In the common usage example above, without a wrapping nullable strategy an empty string would have been passed into the setter causing an exception at this point, the setter being type-hinted with DateTimeInterface or ?DateTimeInterface. In my opinion the error should occur during hydration if the value type is not what you expect from the name of the strategy itself. So raising an exception for the empty string case would even be preferable than allowing it to pass as it is.

Take the BooleanStrategy for instance. Only bool are returned by the hydrate method, otherwise an InvalidArgumentException is raised.

But if you think the idea is not useful, please just close the issue...don't waste too much time on this...
kind regards

[froschdesign]

But maybe I have been not very clear.

If we look at the title and the initial summary of the issue report then yes.

My idea is to "only pass acceptable/consistent hydrating values".

This would certainly end in a BC break because the behaviour is changed. But it would bring consistency to all hydrators if it were implemented that way.

Btw. NullableStrategy was added to allow this behaviour without a BC break.

[pine3ree]

I could add an extra ctor argument (something like .., $strictTypeHydration = false)) and raise an exception for the empty string case, when set to true. This would avoid the BC break. kind regards.

[eugene-borovov]

I believe hydrators should be strict by default. Hydrators only implement the conversion logic. The data preparation logic should be separated from the hydrators. Different data types have different values that are interpreted as empty. For example, the date 0000-00-00 may be empty, or a date with a unix timestamp of 0. In one of my projects, I met the date 2038-01-19 (max unix timestamp) which was considered NULL.

It turns out that you need to maintain several hierarchies of hydrators in parallel, or use a chain of decorators to describe all specific cases. Boolean constructor arguments essentially mean a parallel hierarchy.

[pine3ree]

Hello @eugene-borovov ,

I agree. I meant the extra parameter to be temporary until next major release just to avoid BC break and to allow stricter conversions for those who prefer it.
kind regards

[eugene-borovov]

@pine3ree
I believe this will add a BC break in the future.

I would like to be able to chain strategies like middlewares. Each strategy can interrupt the hydration process and return a value. StrategyChain is almost useless in preparing the value for the target strategy. Therefore, there are new decorators that convert empty values or format dates. In StrategyChain, the extraction and hydration chains are inverted, so this cannot be used to detect empty values.