Fix the query that is generated to purge the old installed extras

[Jako]

What does it do?

Fix incorrect DATETIME value: '0000-00-00 00:00:00' error in the query

Why is it needed?

The generated query is wrong for MySQL installations that have NO_ZERO_DATE/NO_ZERO_IN_DATE activated.

Related issue(s)/PR(s)

MODX 3.x port of #16716

[smg6511]

The question here is: Is it safe to assume that the saved installed value would never be 0000-00-00 00:00:00 (indicating package's status is not installed)? If so, this change is good to go with a few code quality fix ups. If not, the null check should be added as an additional where criteria instead of replacing the original check, right?

[opengeek]

The question here is: Is it safe to assume that the saved installed value would never be 0000-00-00 00:00:00 (indicating package's status is not installed)? If so, this change is good to go with a few code quality fix ups. If not, the null check should be added as an additional where criteria instead of replacing the original check, right?

I believe you are right—the null check needs to be added to the existing criteria rather than replacing it.

[Jako]

I have checked that first before b9b533b but that did not work on at least two installations. Or did I make a mistake in my query?

[opengeek]

I have checked that first before b9b533b but that did not work on at least two installations. Or did I make a mistake in my query?

I think the query was incorrect. The string '0000-00-00 00:00:00" is not the same as 0.

[Jako]

The first check was 'installed:!=' => '0000-00-00 00:00:00' which gives an Incorrect DATETIME value: '0000-00-00 00:00:00' error when NO_ZERO_DATE/NO_ZERO_IN_DATE is activated.

[opengeek]

The first check was 'installed:!=' => '0000-00-00 00:00:00' which gives an incorrect DATETIME value: '0000-00-00 00:00:00' error when NO_ZERO_DATE/NO_ZERO_IN_DATE is activated.

Ohhh… well, that is a huge problem then. I was afraid of that.

[smg6511]

Hmmm, according to the mysql documentation:

For statements such as SELECT that do not change data, invalid values generate a warning in strict mode, not an error.

So, when you ran the query with both criteria, was it a warning or error you were seeing?

[Jako]

The server has the following MySQL 8.0.22 sql_mode set:

STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION

The same sql_mode don't show that issue in MySQL 5.7.44.

The errror message according to SequelPro is: Incorrect DATETIME value: '0000-00-00 00:00:00'

[smg6511]

Yeah, I was just playing with this after changing the mode and got the error as well. Interesting that behavior is not consistent with what the docs say.

It seems to me that the bottom line here is that if someone has these strict date modes enabled in mysql 8+ and there are any zeroish values hanging around in the installed col, things are going to be a bit broken for those rows. It seems like the only way to fix those is manually (they can't be changed via an update query from 0000-00-00 00:00:00 to NULL).

I think we need to consider changing how dates are stored (to unix timestamp) when they can be initially empty or set back to empty (null), as dealing with this sql mode is a PITA.

[Jako]

It is possible with an intermediate value (which normally does not occur in the table):

UPDATE `modx_transport_packages` SET `installed` = "1970-01-02 00:00:00" WHERE `installed` < "1970-03-01 00:00:00";
ALTER TABLE `modx_transport_packages` MODIFY `installed` DATETIME NULL;
UPDATE `modx_transport_packages` SET `installed` = NULL WHERE `installed` = '1970-01-02 00:00:00';

[smg6511]

Ahh, nice! Just ran that (didn't need the ALTER line, because in 3.x that's already the default config for that column) on my dev install. Works.

Maybe this should make it into an upgrade script for the next release?

[opengeek]

Maybe this should make it into an upgrade script for the next release?

Yes. We should review other tables that may have similar issues, as well.

[Jako]

I have used a similar query for repairing the system/context/user settings in some old installations.