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

CRM-20941 Bump Minimum PHP Version to 5.6, Add in pre-upgrade message… #10851

Merged
merged 2 commits into from
Aug 16, 2017

Conversation

seamuslee001
Copy link
Contributor

…s specific to 5.4 and 5.3 and alter Status Check message accordingly

Overview

Increases CiviCRM 4.7 Minimum php level to 5.6 and adds messages accordingly to notify site administrators

Before

Minimum php level was 5.3

After

Minimum php level is not 5.6

Technical Details

N/a
Comments

This also adds some flexibility in for future Minimum PHP release bumps by adding in a constant PREVIOUS_MIN_RECOMMENDED_PHP_VER which can be altered and will present the same style message we are using for php5.5

@seamuslee001
Copy link
Contributor Author

ping @eileenmcnaughton @totten

@eileenmcnaughton
Copy link
Contributor

Looks good - but I think the translation might not be right - @bgm?

$preUpgradeMessage .= ts('You may proceed with the upgrade and CiviCRM %1 will continue working normally, but PHP %2 will not work in releases published after %3. We recommend you use the most recent php version you can', array(
1 => $currentVer,
2 => phpversion(),
3 => $date,
Copy link
Member

Choose a reason for hiding this comment

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

@seamuslee001, please either use a localized date as an argument to the string (using CRM_Utils_Date::formatDate()) or create two separate strings (I prefer the former, even if it sounds more machine-like, admins will read this only once). Translators will not understand why your sentence does not end with a period, or how to conjugate "the end of 2017", if it had a ts.

@seamuslee001
Copy link
Contributor Author

@mlutfy i have tried to localise the date string can you take a look now?

)) .
'</p>';
$preUpgradeMessage .= '<p>';
// CRM-20941 PHP 5.3 end date of End of 2017, PHP 5.4 End date End of Feb 2019 Recommend anyone on PHP 5.5 to move up to 5.6 or later e.g. 7.0
Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 just spotted a minor mistake - 5.4 should say end of Feb 2018

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton fixed @totten i think this is good to go now, Only question is should we put a link in to Tim's blog on the subject. @totten have you email dev with a link to your blog?

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 I think a link to the blog is a good idea

@agileware
Copy link
Contributor

Changing the minimum PHP version really deserves a public announcement to avoid catching people unawares.

@eileenmcnaughton
Copy link
Contributor

Here is the link to the announcement https://civicrm.org/blog/totten/end-of-zombies-php-53-and-54 (people should also have seen that in their news dashboard)

@seamuslee001
Copy link
Contributor Author

@agileware see https://civicrm.org/blog/totten/end-of-zombies-php-53-and-54 which is also in the dashboards.

@eileenmcnaughton
Copy link
Contributor

snap

@seamuslee001
Copy link
Contributor Author

Also this will also alert people via the pre upgrade message and the status page

@agileware
Copy link
Contributor

Cool, thanks for chiming in team 🥇

…s specific to 5.4 and 5.3 and alter Status Check message accordingly
$messages[] = new CRM_Utils_Check_Message(
__FUNCTION__,
ts('This system uses PHP version %1. CiviCRM can be installed on this version, but some specific features are known to fail or degrade. Version %3 is the bare minimum to avoid known issues, and version %2 is recommended.',
ts('This system uses PHP version %1. CiviCRM can be installed on this version, However PHP version %1, will not work in releases published after %2, and version %3 is recommended.',
Copy link
Member

Choose a reason for hiding this comment

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

Comments on grammar: in the message However PHP version %1, will not work, the comma separates the subject ("PHP version %1") and the predicate ("will not work..."). That's weird. Either of these formulations would be more acceptable:

  • Use the comma to set aside the interjection ("However, PHP version %1 will not work...") (my personal preference)
  • Kill the comma ("However PHP version %1 will not work...") (may be OK since "However" is only one word)

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be a full stop

@seamuslee001
Copy link
Contributor Author

@totten @eileenmcnaughton I have added in the link to the blog post and also made that grammatical change as per Tim

@seamuslee001
Copy link
Contributor Author

On Re-evaluation i put in a full stop For Eileen as i think it makes sense now,

@eileenmcnaughton
Copy link
Contributor

thank you!

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton @totten tests have passed now

@eileenmcnaughton eileenmcnaughton merged commit bb03830 into civicrm:master Aug 16, 2017
@eileenmcnaughton eileenmcnaughton deleted the CRM-20941 branch August 16, 2017 23:42
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.

5 participants