Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default value for modManagerLog.occurred column #16526

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

opengeek
Copy link
Member

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

Mark-H and others added 3 commits February 16, 2024 16:02
Add right default value for datatime-type column in modx_manager_log table

Re-up of modxcms#15736 with migration

Satisfy phpcs
@opengeek opengeek added pr/port-to-3 Pull request has to be ported to 3.x. pr/port-to-2 Pull request has to be ported to 2.x. labels Feb 16, 2024
@opengeek opengeek added bug The issue in the code or project, which should be addressed. and removed pr/port-to-2 Pull request has to be ported to 2.x. labels Feb 17, 2024
@opengeek opengeek added this to the v3.0.5 milestone Feb 17, 2024
@Jako
Copy link
Collaborator

Jako commented Feb 20, 2024

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";

@opengeek
Copy link
Member Author

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.

@Jako
Copy link
Collaborator

Jako commented Feb 20, 2024

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.

@opengeek
Copy link
Member Author

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.

@Jako
Copy link
Collaborator

Jako commented Feb 21, 2024

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.

@Ruslan-Aleev Ruslan-Aleev removed the pr/port-to-3 Pull request has to be ported to 3.x. label Feb 22, 2024
opengeek added a commit that referenced this pull request Mar 6, 2024
### 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

### 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
Backport of #16526
@opengeek opengeek merged commit 8a2aabf into modxcms:3.0.x Mar 25, 2024
8 checks passed
opengeek added a commit that referenced this pull request Mar 25, 2024
### 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

---------

Co-authored-by: Mark Hamstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue in the code or project, which should be addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants