Clone DateTimes before returning them from temporal column getters

[rimas-kudelis]

There is a problem in how ActiveRecord currently treats temporal columns. In particular, changes to these columns may be left unnoticed. For example, let's say I have a "Start" column in my table, and I do something like this:

$start = $object->getStart();
$start->modify('+1 day');
$object->setStart($start);
$object->save();

After doing that, I would expect the DB record to be updated, but it is not. And the reason why it is not being updated seems to be the following:

• getStart() returns the original DateTime object, which is stored as a property of $object
• modify() modifies that same DateTime object, without $object noticing the change
• setStart() compares the dates and only changes field value and marks it as modified if the dates are different. Obviously, comparison between references to the same object will always yeld equality, so the column is not marked as modified.
• since no columns are marked as modified, save() does nothing.

(Note I'm actually using Carbon to work with dates, but it is based on DateTime, so I don't think it makes any difference in this case.)

A solution to this would be to either use DateTimeImmutable class, which is new to PHP 5.5, or to clone the DateTime before returning it from the getter. Assuming that Propel allows users to choose their own DateTime implementations for temporal columns, the latter seems to make more sense.

[rimas-kudelis]

It seems that tests provided by #768 almost cover this, but not quite, which is probably why the problem had been left unnoticed.

[rimas-kudelis]

Wow, apparently, my patch (#951) breaks another test which asserts that DateTime is returned as a reference to the original object.

Is this indeed by design? If so, is it really acceptable that ActiveRecord simply ignores changes to its properties, when they just happen to also be objects of some class? To me, this seems completely unexpected.

[staabm]

This PR introduced the return-by-ref behaviour/test https://github.com/propelorm/Propel2/pull/87. Not sure this is the right behaviour but at least it is consistent with other getters returning a object.

That changes dont get picked up by the ActiveRecord sounds like a bug to me

[rimas-kudelis]

Thanks! Huh, #87 supposedly fixes #59, which has the following example of an intuitive API in the initial comment:

$foo->getDate()->modify('+1 days'); $foo->save();

which is exactly the thing that is currently not working... :)

I see another possible solution to this problem: to have two protected DateTime instances for each temporal column: one for change tracking purposes only (it should only be updated during save()), and the other one for using with getters/setters. This would mean less cloning, but I'm not convinced this is intuitive and expected. When I use a getter on a scalar property, I can transform the result as I like, but still have to use the setter to feed it back to the object. Having temporal columns act differently seems to me inconsistent at the very least.

[staabm]

It behaves consitantly like using foreign objects on the model. So as soon as we represent dates as objects I would expect them to be returned by ref. As those objects are not scalar values thats the way php works.

@marcj wdyt?

[rimas-kudelis]

There is an explanation by the author of DateTime why the API of this class is not exactly intuitive: http://derickrethans.nl/immutable-datetime.html . But even if we assume it's all good with this class, I don't think it's OK and consistent that when the DateTime object changes, ActiveRecord does not notice the change and does not mark the column as changed, even if I call the column setter explicitly afterwards (which shouldn't be required, according to the code example above).

And the setter does not see the change because it compares its argument to a DateTime instance that might have already been modified externally, because the getter returns it by reference. I would say it makes change tracking of temporal columns quite unreliable in its current state.

Regardless of all this, if breaking this bit of compatibility with older versions is not an option, perhaps the getter could then be extended to accept an additional argument telling it to return a clone instead of the original DateTime object?

[staabm]

We dont discuss a BC break because DateTime was introduced in Propel2 which hasnt seen a release yet. So we are still free to break things here..

[rimas-kudelis]

That's good to know!

I've added a test in 9797752c762ea2c2d1a895c425c5435cd2081f6f to better illustrate my point about what is wrong at the moment.

[rimas-kudelis]

So, who should have the final say about how we should proceed from here?

[janwalther]

Also Propel1 has been using PHP's internal DateTime class when configured with the following in propel.ini:

# Enable full use of the DateTime class.
# Setting this to true means that getter methods for date/time/timestamp
# columns will return a DateTime object when the default format is empty.
propel.useDateTimeClass = true

# Specify a custom DateTime subclass that you wish to have Propel use
# for temporal values.
propel.dateTimeClass = DateTime

Meanwhile also Derick Rethans admitted that it was a mistake to design DateTime to be mutable (because value objects should never be mutable). Therefore PHP introduced the DateTimeImmutable class. Propel2 uses DateTime as default for not breaking BC. But you can easily change the generator.dateTime.dateTimeClass variable to "DateTimeImmutable" when you want to use immutable DateTime objects.

[dereuromark]

Is this still relevant or can it be closed?

[bugos]

I think this is still relevant and should be fixed. I came here after debugging an issue caused by me modifying the mutable DateTime object that Propel returns.

At least the default value for generator.dateTime.dateTimeClass should be set to DateTimeImmutable in Propel2.

[dereuromark]

Are you able to make a new PR here?