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

dev/core#1782 Set activity_date_time and created_date to default to current_timestamp #17450

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Jun 1, 2020

Overview

Add defaults for activity_date_time and created_date if not set.

Before

No default.

After

Default

Technical Details

Comments

Found via #17274

@seamuslee001 I think we might need an upgrader step too so the database gets the defaults defined?

@civibot
Copy link

civibot bot commented Jun 1, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Jun 1, 2020

I think the tests will do a good job of finding any issues here. Once the upgrade change is in too then then I'm happy when jenkins is (MOP was premature as the upgrade change not in yet)

@seamuslee001
Copy link
Contributor

The failure was

mysql: [Warning] Using a password on the command line interface can be insecure.
ERROR 1064 (42000) at line 4202: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DEFAULT CURRENT_TIMESTAMP COMMENT 'Date and time this activity is scheduled to o' at line 8

Also note this

"Previously, at most one TIMESTAMP column per table could be automatically initialized or updated to the current date and time. This restriction has been lifted. Any TIMESTAMP column definition can have any combination of DEFAULT CURRENT_TIMESTAMP and ON UPDATE CURRENT_TIMESTAMP clauses. In addition, these clauses now can be used with DATETIME column definitions. For more information, see Automatic Initialization and Updating for TIMESTAMP and DATETIME."

https://dev.mysql.com/doc/relnotes/mysql/5.6/en/news-5-6-5.html So this would break anyone who was trying to install on MySQL 5.5 (no of course deprecated but we still permit installation on it)

@eileenmcnaughton
Copy link
Contributor

@colemanw any thoughts?

@colemanw
Copy link
Member

colemanw commented Jun 3, 2020

It sounds like we'll have to drop support for MySql 5.5 before releasing this.
That's fine with me but the question is how much lead time or warning should we give?
Looks like only last month we bumped the minimum from 5.1 to 5.5!
According to http://stats.civicrm.org/?tab=technology most sites are already up to or beyond 5.6, with about a thousand sites still running 5.5 (those same sites may well be running an older version of Civi as well).

@mattwire mattwire force-pushed the activitydefaults branch from 0a94069 to a734ca1 Compare June 3, 2020 13:26
@colemanw
Copy link
Member

colemanw commented Jun 3, 2020

@mattwire the DAO still needs regen.
We'll also need to put in some upgrade sql but need to pick a target version first.
If we merge this in June then it goes into the RC in July and released in August; we'd just need to put the word out btw now and then that we are dropping 5.5 support.
We should also bump up CRM_Upgrade_Incremental_General::MIN_INSTALL_MYSQL_VER as part of this PR.

@colemanw
Copy link
Member

colemanw commented Jun 4, 2020

@eileenmcnaughton
Copy link
Contributor

We discussed this & plan is remove mysql 5.5 support in 5.28 - so with some test changes & changes to support that we will be able to merge this

@mattwire mattwire force-pushed the activitydefaults branch from a734ca1 to b348dc2 Compare June 6, 2020 14:40
@mattwire
Copy link
Contributor Author

mattwire commented Jun 6, 2020

@colemanw @eileenmcnaughton I've added the upgrader and the min SQL version. And regenerated DAO. Are we good to merge?

@colemanw
Copy link
Member

colemanw commented Jun 6, 2020

Looks great!

@eileenmcnaughton
Copy link
Contributor

I removed the ready for review as this is stale. I'm not a fan of the label since in theory they should all be if they are in the review queue

@mattwire mattwire force-pushed the activitydefaults branch from b348dc2 to 487813a Compare June 8, 2020 11:20
@mattwire
Copy link
Contributor Author

mattwire commented Jun 8, 2020

@colemanw This is now rebased following DAO updates.

@eileenmcnaughton
Copy link
Contributor

If this doesn't pass it might be that we need to adjust CI - @seamuslee001 emailed the dev list to highlight that we will discontinue 5.5 mysql support

@seamuslee001 seamuslee001 merged commit 46662be into civicrm:master Jun 8, 2020
@eileenmcnaughton
Copy link
Contributor

nice!

@colemanw
Copy link
Member

colemanw commented Jun 9, 2020

Nice work everyone. And thanks for the followups #17496 and #17558 @seamuslee001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants