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 modManagerLog MySQL > 5.7.8 #15736

Closed
wants to merge 3 commits into from
Closed

Fix modManagerLog MySQL > 5.7.8 #15736

wants to merge 3 commits into from

Conversation

catsmeatman
Copy link
Contributor

@catsmeatman catsmeatman commented Jun 16, 2021

What does it do?

Add right default value for datatime-type column in modx_manager_log table

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. In this strict modes datatime default value cannot be NULL and must be > '0000-00-00'.

How to test

When setup modx In MySQL 5.7.8 not create index user_occurred at modx_manager_log table.

Try to add index in console
create index user_occurred ON modx_manager_log (user, occurred)

Result
ERROR 1067 (42000): Invalid default value for 'occurred'

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Jun 16, 2021
@Ibochkarev Ibochkarev requested a review from sergant210 June 26, 2021 07:44
@Ibochkarev Ibochkarev added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Jul 28, 2021
@Ibochkarev Ibochkarev added this to the v2.8.4 milestone Jul 28, 2021
@Jako
Copy link
Collaborator

Jako commented Jul 29, 2021

There could be an update script to modify that table column for existing installations too.

Copy link
Member

@opengeek opengeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an update script to correct existing installations before being accepted.

@opengeek opengeek removed the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Sep 23, 2021
@JoshuaLuckers
Copy link
Contributor

@catsmeatman would it be possible for you to create an upgrade script?

@catsmeatman
Copy link
Contributor Author

@JoshuaLuckers where can I read how to do this?

@Jako
Copy link
Collaborator

Jako commented Feb 1, 2022

This can be done like this: https://github.com/modxcms/revolution/blob/2.x/setup/includes/upgrades/mysql/2.0.4-pl.php

You have to create a file 2.8.4-pl.php with some ALTER TABLE code.

@catsmeatman
Copy link
Contributor Author

@Jako Thx, I'll do it this weekend

@Ibochkarev
Copy link
Collaborator

@catsmeatman Please make the suggested edits @Jako

@opengeek opengeek modified the milestones: v2.8.4, v2.8.5 Apr 28, 2022
@opengeek opengeek modified the milestones: v2.8.5, v2.8.6 Mar 7, 2023
@opengeek opengeek modified the milestones: v2.8.6, v2.8.7 Sep 28, 2023
Mark-H added a commit to Mark-H/revolution that referenced this pull request Feb 10, 2024
Add right default value for datatime-type column in modx_manager_log table

Re-up of modxcms#15736 with migration
@Mark-H
Copy link
Collaborator

Mark-H commented Feb 10, 2024

As this PR has gone stale I've re-created it for 3.x in #16520.

@Mark-H Mark-H closed this Feb 10, 2024
Mark-H added a commit to Mark-H/revolution that referenced this pull request Feb 10, 2024
Add right default value for datatime-type column in modx_manager_log table

Re-up of modxcms#15736 with migration

Satisfy phpcs
@rthrash
Copy link
Member

rthrash commented Feb 16, 2024

@Mark-H will you back port this for 2.8.7?

@opengeek
Copy link
Member

@Mark-H will you back port this for 2.8.7?

@rthrash — I will handle it. I'm going to make some changes to the PR anyway.

opengeek pushed a commit to opengeek/revolution that referenced this pull request Feb 16, 2024
Add right default value for datatime-type column in modx_manager_log table

Re-up of modxcms#15736 with migration

Satisfy phpcs
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 removed this from the v2.8.7 milestone Mar 14, 2024
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 <hello@markhamstra.com>
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 <hello@markhamstra.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants