(dev/drupal#79) Fail more gracefully when upgrading on PHP 5.x #583
Conversation
|
(Standard links)
|
This version requirement officially went up to PHP 7.0 circa Civi 5.14.
However, at that time, the upgrade metadata was kept at PHP 5.6 to allow
somewhat softer landing for stragglers. That's no longer possible in Civi
5.16+,
This just gives a clearer error when someone tries to upgrade with PHP 5.x.
Before
------
When upgrading via drush or Drupal web UI on PHP 5.6, the Civi class-loader fails to initialize.
```
Parse error: syntax error, unexpected ':', expecting '{' in /Users/totten/bknix/build/dmaster/web/sites/all/modules/civicrm/vendor/league/csv/src/functions.php on line 33
```
(Approximate call-path: `civicrm.drush.inc | civicrm.module` =>
`civicrm.settings.php` => `CRM_Core_ClassLoader` => `vendor/autoload.php` =>
`vendor/league/csv/src/functions.php`)
After
-----
When upgrading via drush on PHP 5.6, the error points to the actual problem.
```
[bknix-old:~/bknix/build/dmaster/web/sites/all/modules/civicrm] drush civicrm-upgrade-db
CiviCRM requires PHP 7.0.0+. Drush is running PHP 5.6.38. [error]
```
When upgrading via web UI on PHP 5.6, the error message is similar:
```
CiviCRM requires PHP 7.0.0+. The web server is running PHP 5.6.38.
```
(Note: I tweaked the text to emphasize that the PHP version is determined by
the "drush" or "web server" environment - in some systems, these are
different environments with different PHP versions. A phrase like "your
version" can be confusing/misleading in those cases.)
Comments
--------
The canonical representation of the minimum PHP version is in
`$civicrm_root/CRM/Upgrade/Form.php`. However, correctly reading that
metadata requires loading `civicrm.settings.php`, which triggers the crash.
To work around this, we reproduce the constant and use a unit-test to ensure
its continued accuracy.
Also, it seems useful to put the same metadata in `civicrm.info`. I'm not
sure if D7 uses this in a meaningful way, but it's good to be accurate.
totten
force-pushed
the
7.x-5.17-php-min
branch
from
9990b6f to
1b62302
Compare
|
Thanks @totten. The wording change about "web/drush" vs. "your" is great an on-point, but I wonder if that would actually inform people who don't know that you can possibly have two different ones (i.e. they've installed drupal first and it's telling them it's php 7 via web status report, then they go use drush). In the web server case it's fine, but in the drush case, it might be useful to append instructions like "It is possible your command line is configured to use a different php version than the web server. Check your hosting control panel settings to confirm you are using php 7 in all configurations." |
|
@totten test failures relate |
|
Off topic but I got pinged by it so of course I looked (personality flaw). Curious that tests are running phpunit4. Expected? |
|
Possibly a job config not updated maybe? |
|
jenkins, test this please |
|
Good catch on phpunit4. Filed civicrm/civicrm-buildkit#467 and https://lab.civicrm.org/dev/core/issues/1194 |
|
Regarding a longer message - I'm inclined to abstain on that one and invite you to do a PR with better text. (The current draft makes it slightly better than before - your points could make it better-er.) Aesthetically, I'm less keen on that style of message (e.g. messages about externalities don't age as well)... but, pragmatically, you're quite right in identifying a segment of the audience for which that's a good time for that discussion. Maybe it'd make sense to output a link to sysadmin docs which give the bigger picture about system requirements? (Presumably, the docs would be maintained more sensitively than this message would?) |
|
Ok. I can test this. |
…P 5.x Before ------ If an admin stracts the code and navigates to `/sites/all/modules/civicrm/install/index.php`, it displays a syntax error. After ----- If an admin stracts the code and navigates to `/sites/all/modules/civicrm/install/index.php`, it displays the message: > __PHP Version Requirement__ > CiviCRM requires PHP 7.0+. The web server is running PHP 5.6.38. Comments -------- This is similar to civicrm/civicrm-drupal#583 The canonical representation of the minimum PHP version is in `$civicrm_root/CRM/Upgrade/Form.php`. However, setting up the classloader triggers a syntax error, so we need to read this without having access to the classloader. The approach herein has a few effects: * The minimum PHP can be read from a JSON file. * That JSON file is also used by `composer`, so you'll also get better errors when downloading that way. * At some unknown point, the minimum will probably bump up again (7.1 or 7.2 or whatever). When that happens, the unit-test will ensure we keep `CRM/Upgrade/Form.php` and `composer.json` in sync. Note: I was little concerned that the `composer.json` file might not be available when normal installers run, so I checked the published tarballs for D7, BD, WP, and J - in all cases, the `composer.json` looks to be included at the expected location.
|
On Mattermost, Dave reported successful testing of the messages for upgrade use-cases. He also identified an issue with the install experience, for which I submitted a separate revision - civicrm/civicrm-core#15089 |
…P 5.x Before ------ If an admin stracts the code and navigates to `/sites/all/modules/civicrm/install/index.php`, it displays a syntax error. After ----- If an admin stracts the code and navigates to `/sites/all/modules/civicrm/install/index.php`, it displays the message: > __PHP Version Requirement__ > CiviCRM requires PHP 7.0+. The web server is running PHP 5.6.38. Comments -------- This is similar to civicrm/civicrm-drupal#583 The canonical representation of the minimum PHP version is in `$civicrm_root/CRM/Upgrade/Form.php`. However, setting up the classloader triggers a syntax error, so we need to read this without having access to the classloader. The approach herein has a few effects: * The minimum PHP can be read from a JSON file. * That JSON file is also used by `composer`, so you'll also get better errors when downloading that way. * At some unknown point, the minimum will probably bump up again (7.1 or 7.2 or whatever). When that happens, the unit-test will ensure we keep `CRM/Upgrade/Form.php` and `composer.json` in sync. Note: I was little concerned that the `composer.json` file might not be available when normal installers run, so I checked the published tarballs for D7, BD, WP, and J - in all cases, the `composer.json` looks to be included at the expected location.
This version requirement officially went up to PHP 7.0 circa Civi 5.14. However, at that time, the upgrade metadata was kept at PHP 5.6 to allow somewhat softer landing for stragglers. That's no longer possible in Civi 5.16+,
This just gives a clearer error when someone tries to upgrade with PHP 5.x.
Depends: civicrm/civicrm-core#15082
Before
When upgrading via drush or Drupal web UI on PHP 5.6, the Civi class-loader fails to initialize.
(Approximate call-path:
civicrm.drush.inc | civicrm.module=>civicrm.settings.php=>CRM_Core_ClassLoader=>vendor/autoload.php=>vendor/league/csv/src/functions.php)After
When upgrading via drush on PHP 5.6, the error points to the actual problem.
When upgrading via web UI on PHP 5.6, the error message is similar:
(Note: I tweaked the text to emphasize that the PHP version is determined by the "drush" or "web server" environment - in some systems, these are different environments with different PHP versions. A phrase like "your version" can be confusing/misleading in those cases.)
Comments
The canonical representation of the minimum PHP version is in
$civicrm_root/CRM/Upgrade/Form.php. However, correctly reading that metadata requires loadingcivicrm.settings.php, which triggers the crash.To work around this, we reproduce the constant and use a unit-test to ensure its continued accuracy.
Also, it seems useful to put the same metadata in
civicrm.info. I'm not sure if D7 uses this in a meaningful way, but it's good to be accurate.