revolution icon indicating copy to clipboard operation
revolution copied to clipboard

Fix default value for modManagerLog.occurred column

Open opengeek opened this issue 1 year ago • 5 comments

What does it do?

Adds a default value for the datetime column in the modx_manager_log table that is compatible with strict modes, which may be enabled in some environments.

Re-up of #15736 with migration Re-up of #16520 to target 3.0.x

Why is it needed?

Beginning with MySQL > 5.7.8 added strict modes ERROR_FOR_DIVISION_BY_ZERO, NO_ZERO_DATE, NO_ZERO_IN_DATE. With these strict modes enabled, a datetime default value cannot be NULL and must be > '0000-00-00'.

How to test

Install/update on MySQL 5.7.8

Related issue(s)/PR(s)

Re-up of #15736 Re-up of #16520

opengeek avatar Feb 16 '24 23:02 opengeek

It is nice to have NULL available in datetime columns. This is allowed with NO_ZERO_DATE and NO_ZERO_IN_DATE and in strict mode. Only 0000-00-00 is not permitted.

MODX has some datetime columns with NULL allowed i.e. in the editedon column of the system settings.

The following sql commands are working fine here to solve some similar issues in other MODX datebase columns. The temporary date changes are maybe not 100% correct but normally a date in the 1970s does not exist in those columns in MODX.

UPDATE `modx_system_settings` SET `editedon` = "1970-01-02 00:00:00" WHERE `editedon` < "1970-03-01 00:00:00";
ALTER TABLE `modx_system_settings` CHANGE `editedon` `editedon` TIMESTAMP NULL DEFAULT NULL;
UPDATE `modx_system_settings` SET `editedon` = NULL WHERE `editedon` < "1970-03-02 00:00:00";

Jako avatar Feb 20 '24 15:02 Jako

It is nice to have NULL available in datetime columns. This is allowed with NO_ZERO_DATE and NO_ZERO_IN_DATE and in strict mode. Only 0000-00-00 is not permitted.

MODX has some datetime columns with NULL allowed i.e. in the editedon column of the system settings.

The following sql commands are working fine here to solve some similar issues in other MODX datebase columns. The temporary date changes are maybe not 100% correct but normally a date in the 1970s does not exist in those columns in MODX.

UPDATE `modx_system_settings` SET `editedon` = "1970-01-02 00:00:00" WHERE `editedon` < "1970-03-01 00:00:00";
ALTER TABLE `modx_system_settings` CHANGE `editedon` `editedon` TIMESTAMP NULL DEFAULT NULL;
UPDATE `modx_system_settings` SET `editedon` = NULL WHERE `editedon` < "1970-03-02 00:00:00";

While this may tangentially be related to this PR, this proposal belongs in a separate issue or should be prepared in a separate PR. The modManagerLog.occurred column specifically should NOT allow NULL values.

opengeek avatar Feb 20 '24 15:02 opengeek

The issue with the migration script can be invalid existing values in the column, if I am right. In that case the database column definition can't be changed and an SQL error occurs.

Jako avatar Feb 20 '24 15:02 Jako

The issue with the migration script can be invalid existing values in the column, if I am right. In that case the database column definition can't be changed and an SQL error occurs.

I believe the only issue was that the table could not be created in a system with these strict_mode options enabled. I don't believe there are cases of actual invalid values being inserted into the table, are there? If there are, we can certainly attempt to resolve them first, but I don't think this is an actual issue.

opengeek avatar Feb 20 '24 16:02 opengeek

According to the docs https://dev.mysql.com/doc/refman/8.0/en/sql-mode.html#sqlmode_no_zero_date only 0000-00-00 is not permitted. I have not checked, whats going wrong if the column has no value in xPDO. Is it prepared as 0000-00-00 in that case by xPDO?

In this table it does not make sense to have a NULL value available. So I am fine with the patch.

Jako avatar Feb 21 '24 08:02 Jako