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#1681 Add in deprecation notice for Systems using MySQL versions before 5.7 and require 5.5 for install #17261

Merged
merged 2 commits into from
May 15, 2020

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented May 8, 2020

Overview

This PR adds in a system check for CiviCRM installs using MySQL versions before 5.7. This also sets the minimum install version to be 5.5 but we could make it 5.7 instead if preferred.

See also: https://lab.civicrm.org/dev/core/-/issues/1681

Before

No status check around MySQL version and minimum install version if MySQL 5.1

After

status check around MySQL version and minimum install version if MySQL 5.5

ping @eileenmcnaughton @totten

@civibot
Copy link

civibot bot commented May 8, 2020

(Standard links)

@civibot civibot bot added the 5.26 label May 8, 2020
@totten
Copy link
Member

totten commented May 8, 2020

Initially got some warnings about a bad array_merge() via CRM_Utils_Check_Component::checkAll(), but a small patch fixed:

diff --git a/CRM/Utils/Check/Component/Env.php b/CRM/Utils/Check/Component/Env.php
index dfa4b0ff5a..1995053efd 100644
--- a/CRM/Utils/Check/Component/Env.php
+++ b/CRM/Utils/Check/Component/Env.php
@@ -952,6 +952,7 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component {
         'fa-server'
       );
     }
+    return $messages;
   }

 }

I ran this on mysql55 (ie a supported version that's being phased-out/deprecated), and it said that I needed to "upgrade now", which felt incorrect. It looks like this patch used some of the checkPhpVersion() messaging as a starting point -- which is a sensible starting point -- but it only copied some of it.

In checkPhpVersion(), note that it displays different messages for sites in each of these situations:

  1. Supported version (ie >=RECOMMENDED)
  2. Deprecated version (ie <RECOMMENDED but >=MIN_INSTALL)
  3. Unsupported version (ie <MIN_INSTALL)

In checkPhpVersion(), the "upgrade now" wording was used (3) unsupported versions... but on a deprecated version, you'd get softer language.

I think the checkMysqlVersion() should make more distinctions like checkPhpVersion().

(Note: If you think the "deprecated" message is a little soft, then maybe we should wordsmith it a bit. But I think it's proper to have different messages for systems with deprecated vs unsupported.)

@mattwire mattwire added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label May 10, 2020
@eileenmcnaughton
Copy link
Contributor

Just noting that while I think we should make it clear 5.5 & 5.6 are unsupported (per docs) I think the scope of this PR should be limited to technical consistency around < 5.5 - which should be uncontroversial

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton @totten I have made changes to fix the test failures and also to have the check only activate if less than min install which would be 5.5

@eileenmcnaughton eileenmcnaughton added merge on pass and removed needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state labels May 14, 2020
@eileenmcnaughton
Copy link
Contributor

OK - this addresses the things that were blocking this PR being merged. As discussed we should discuss further the best setting & whether we should also tell people if they are below the recommended version

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

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