Removing zeroed dates

[Seb-C]

This removes the default dates 0000-00-00 00:00:00 and replaces it with NULL, since 0000-00-00 it's not a valid date NOR recognized by any API.

[shaoshiva]

It would be better to use the explicit syntax NULL DEFAULT NULL instead of the implicit syntax NULL for readability.

[Meroje]

👍 the zero date is also refused by strict mode

[shaoshiva]

This will break all the features that compare dates lesser than something.

Take this query for example :

SELECT * FROM `test` WHERE date < '2016-04-22'

It will match the rows with a date whose value is 0000-00-00 00:00:00 but not those whose value is NULL.

[Seb-C]

I don't think an empty date should be comparable with another. Plus, this can't break existing apps since it will only affect future migrations.

[shaoshiva]

You PR affect the fields used by the updated/created observer, so there may be features that rely on it and that could make comparison in a query.

I'm just saying that this could have side effects, nothing else.

[Meroje]

No breaking change though (and the behaviour you mention will set valid dates).

I could find code that has to check for zeroed dates in addition to null, but nothing expecting valid work from it

[shaoshiva]

Ok, here is an example.

I developped a website using the app wizard, dates fields were created with a default empty value, then I developped a behaviour to find all the items that were imported before :

$item->find_imported_before();

The method executes a query with a condition on the "imported_at" field :

$this->query(array(
  'where' => array(
    array('imported_at', '<', $this->imported_at),
  )
));

This will return all the items with a date lesser than the date on the current item and thoses with an empty date, that's how mysql works.

Then you patch the App Wizard with this PR. Now If I want to create a new application with a model that implements this behaviour, it won't work as expected, because dates with NULL values won't be returned.

Breaking change.

[Meroje]

Your behavior is confusing, expect bugs. I would expect items without a valid import date to not have been imported, thus I also do not expect to find them with find_imported_before()

[Seb-C]

This may break future developments using the behaviour, but it won't break already running apps.
Plus, this will allow you to further improve the code since this kind of comparison without any checks is dangerous and semantically wrong.
And this kind of code seems unlikely to me since it won't work with migrations created without the app wizard nor with many databases.

[Meroje]

I understand the generated app is breaking compatibility now, but there is no issue with that right ?

[shaoshiva]

👍 if published in a major release and/or a note that indicates this change

[Meroje]

👍