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/drupal#79) Fail more gracefully when attempting to install on PHP 5.x #15089

Merged
merged 2 commits into from
Aug 22, 2019

Conversation

totten
Copy link
Member

@totten totten commented Aug 21, 2019

See also: https://lab.civicrm.org/dev/drupal/issues/79

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 future time, 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.

…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.
@civibot
Copy link

civibot bot commented Aug 21, 2019

(Standard links)

@demeritcowboy
Copy link
Contributor

As noted in civicrm/civicrm-drupal#583 I had tested this along with that.

Note that if you try to run composer install (with php 7) it says "Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them." Does the lock file also need updating or is that done as part of a separate packaging process?

Separate note that isn't really relevant in 2019: While looking into some local issues during testing it came up that this adds a use of json_decode before it checks that json support is available in php (https://github.com/civicrm/civicrm-core/blob/5.16.2/install/index.php#L749). Unlikely to be a problem.

@totten
Copy link
Member Author

totten commented Aug 21, 2019

@demeritcowboy Thanks. Yeah, normally when changing the composer.json, one needs to run composer update <foo/bar> to get the new code and to update the .lock. I didn't do that because requires: php: ... isn't a normal, downloadable package. But, sure enough, composer update php does revise the .lock file. I pushed an update for that.

For json_exists()... yeah, agree it's not much of an issue in 2019. I started throwing up a patch anyway

-if (file_exists($composerJsonPath)) {
+if (file_exists($composerJsonPath) && function_exists('json_decode')) {

but then considered the user's journey a bit more. If you're on some old version of PHP where it was common to be missing json_decode, then:

  • Continuing through to the alternative syntax error about { and : will be much more obscure.
  • The error Call to undefined function json_decode() should be imminently google-able.
  • Suppose, alternatively, we added another check (if !function_exists('json_decode'), then show an error about needing json_decode), our error would be substantively the same as PHP's default error (and less google-able).

@demeritcowboy
Copy link
Contributor

Makes sense. I suppose the alternate error if function_exists('json_decode') fails could just be "CiviCRM requires PHP 7.0+. The web server is running PHP 5.1.0.", which is technically not the failure reason but gets them there faster. Anyway it seems to be enough as-is and thanks you've done a lot on this issue already.

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 & I discussed & feel this is better in than out & it would be fine to push out 5.16 with this in it

@eileenmcnaughton eileenmcnaughton merged commit c380cf7 into civicrm:5.17 Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants