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

N°3253 Fix setup crashing on incompatible PHP versions #155

Open
wants to merge 10 commits into
base: develop
from

Conversation

@piRGoif
Copy link
Member

piRGoif commented Aug 14, 2020

Context

A couple of time after adding some PHP code in iTop core, we saw that the setup cannot be launched anymore when using old PHP versions.

Some past examples :

  • adding a finally block in \MetaModel::GetObjectWithArchive (commit 0adadeb in iTop 2.6.0) made iTop setup incompatible with PHP < 5.5.0
  • adding constant visibility in ormLinkSet (commit b58a084 in iTop 2.8.0 dev version) broke setup for PHP < 7.1.0

Setup homepage error

Sample error when opening the setup with an incompatible PHP version :

parse error on setup homepage

🤯

We should instead display a clear message stating that the current PHP version is incompatible with iTop !

Risks

  • This is something we don't test for now (we are aiming to do so but this is complicated as docker images with such old PHP version are hard to find/maintain !)
  • Next major iTop version will update min PHP version from 5.6.0 to 7.1.3, and as this is directly configured in dev team PHPStorm's we will introduce more and more incompatible code in iTop core !

Proposition

This PR propose a lightweight setup index that will first check PHP version. This new index page includes the bare minimum files to avoid PHP parse errors.

If the test succeeds the setup is launched, without changing the browser's URL.
If not an error message is displayed.

Refactorings

Two new files were created, they are the only dependencies for the new setup/index.php :

  • setup/setuputilslight.class.php
  • setup/setupconst.class.php

The below constants/methods were moved/introduced :

  • SetupUtils const for PHP / MySQL versions
  • SetupUtils::CheckPhpVersion method and CheckResult object
  • WebPage::PAGES_CHARSET
  • \WebPageLight::EmbedSetupPageContent
  • ITOP_* const that were defined in core/config.class.inc.php
@piRGoif piRGoif added this to the 2.8.0 milestone Aug 14, 2020
@piRGoif piRGoif self-assigned this Aug 14, 2020
Copy link
Contributor

dflaven left a comment

J'aime bien l'idée de protéger le setup contre un problème de PHP, mais est-ce qu'on peut pas faire un truc plus simple et qui permettrait aussi de se protéger contre une installation où PHP ne fonctionne même pas? Je pensais à une page index.php qui soit essentiellement du HTML avec juste la détection de version de PHP qui produirait un markup qu'on vérifie en JS. Si c'est bon, on passe à la page du setup proprement-dit, sinon on affiche (toujours en JS) un message indiquant que PHP n'est pas configuré ou n'est pas la bonne version...

application/xmlpage.class.inc.php Show resolved Hide resolved
core/ormlinkset.class.inc.php Show resolved Hide resolved
@piRGoif
Copy link
Member Author

piRGoif commented Aug 15, 2020

Thanks Denis for the feedback, your solution is simplier indeed : a simple index that generated a string in PHP, and controlling what was generated server side by a JS code.

Implementing this we shouldn't forget to check also PHP version (maybe generate a php -v server side ?) as the root of the problem was parse errors generated on the setup homepage.
I like that during the setup the URL displayed by the browser don't change, it isn't mandatory but I think we should keep it this way... Might be hard to implement in your proposed solution ?

@bruno-ds
Copy link
Member

bruno-ds commented Aug 16, 2020

Php expose it's version through this collection of constants:
https://www.php.net/manual/fr/reserved.constants.php

Ie: PHP_VERSION

@piRGoif piRGoif force-pushed the feature/3253_setup_php_version_check branch from 9188208 to a802264 Sep 7, 2020
@piRGoif piRGoif requested a review from rquetiez Sep 18, 2020
@piRGoif piRGoif force-pushed the feature/3253_setup_php_version_check branch from a802264 to 1d65073 Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.