joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.77k stars 3.65k forks source link

[4.0] Install: Incorrect datetime value #16788

Closed 810 closed 5 years ago

810 commented 7 years ago

Steps to reproduce the issue

install from 4.0 branch

Expected result

Finish the installation

Actual result

Incorrect datetime value: '0000-00-00 00:00:00' for column 'lastResetTime' at row 1

System information (as much as possible)

Additional comments

csthomas commented 6 years ago

I checked the code and there is other solution implemented.

https://github.com/joomla-framework/database/blob/ac02265f2afa271eec1b146198c9baba8a3c6397/src/Mysqli/MysqliDriver.php#L927-L936

But there is no solution for the PDO driver yet.

mbabker commented 6 years ago

I wish we would actually start using proper null values in our data model instead of having everything NOT NULL DEFAULT ''. The concept of a "null date", especially one that has a variable value, is problematic at best.

csthomas commented 6 years ago

If we can change '0000-00-00 00:00:00' to '1000-01-01 00:00:00' then maybe we should do it in a better way and change it to NULL.

csthomas commented 6 years ago

Usage of $query = ...$db->quote($db->getNullDate())... should be then deprecated and joomla core should use for insert/update query code like column = NULL directly.

What about backport $query->isNullDatetime() to J3.10?

mbabker commented 6 years ago

That null date stuff needs to get into 3.x, there are enough problems with MySQL 5.7+ caused by that 0000 date that can't be addressed well without potential B/C issues in the data model. But for 4.0, we should do it right.

csthomas commented 6 years ago

I know we can break B/C in J4 but I read somewhere that an extension that works on J3.10 also should work on J4.0.

I would like to backport $query->isNullDatetime() to J3.10 or even to J3.9 in order to allow developer to write code which will work on J4 too. $db->getNullDate() will be deprecated in J4 and not used by core extenions. $db->getNullDate() may return '0000-00-00 00:00:00' or '1000-01-01 00:00:00' but the general advice will be to use NULL in J4.

In short: In WHERE ... clause we should use $query->isNullDatetime('column_name'). In SET (UPDATE/INSERT) clause we will use column = NULL in J4 and column = . $db->getNullDate()` for J3 as now.

Maybe for J4 $db->getNullDate() should return raw null and $db->quote(null) should return text 'NULL'.

csthomas commented 6 years ago

@brianteeman At the moment, this problem does not exist, but I am not convinced that it is correct.

After database framework overrides sql_mode and use only,

$sqlModes = [
            'STRICT_TRANS_TABLES',
            'ERROR_FOR_DIVISION_BY_ZERO',
            /* 'NO_ZERO_DATE', - this only will be added by mysql 5.7.4 - 5.7.7 and >= 8.0 implicitly */
            /* 'NO_ZERO_IN_DATE', - this only will be added by mysql 5.7.4 - 5.7.7 and >= 8.0 implicitly */
            'NO_AUTO_CREATE_USER',
            'NO_ENGINE_SUBSTITUTION',
        ];

with merged PR https://github.com/joomla-framework/database/pull/120 this issue has been resolved.

[UPDATED]

From MySQL 5.7.4 through 5.7.7 and for >= 8.0 NO_ZERO_DATE is included in 'STRICT_TRANS_TABLES'.

csthomas commented 6 years ago

I wrote an example for module table at https://github.com/joomla/joomla-cms/pull/21901 how to replace '0000-00-00 00:00:00' by NULL.

This is the version in which I force user data change (publish_up,publish_down, check_out_time), when it will be a real joomla update, but it will not work if you manually change files, and then click 'Fix' to update database schema.

brianteeman commented 5 years ago

As #21901 has now been merged I assume that it needs to be fixed in other places as well?

wilsonge commented 5 years ago

Yup

alikon commented 5 years ago

for com_contacts #24675

Quy commented 5 years ago

Please see #24535 and test related PRs.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16788.