Simplify PHP version error message in index.php, install.php, api.php…

[justinbeaty]

Description (*)

The hardcoded HTML in some root files has always bothered me. First, it contains broken html: (two closing </a>)

<a href="https://www.openmage.org/magento-lts/install.html" target="">Find out</a> how to install</a>

But also, we really don't need those inline styles. It looks fine considering nobody should see it.

I also created the $minVersion variable so we don't have to change it twice in each file.

Manual testing scenarios (*)

1. Change $minVersion to 9.0.0 or similar to see the error message.

Questions or comments

Can we move $minVersion to some file that gets included so we don't have to duplicate it?

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[sreichel]

Can we move $minVersion to some file that gets included so we don't have to duplicate it?

I guess until Mage:app() isnt called, we cannot.

[justinbeaty]

Can we move $minVersion to some file that gets included so we don't have to duplicate it?

I guess until Mage:app() isnt called, we cannot.

I was thinking like:

config.php:

<?php
$minVersion = '7.3.0';

and in [index|install|api|get].php:

require_once __DIR__ . '/config.php';
if (version_compare(phpversion(), $minVersion, '<') === true) {

But, it's kind of pointless to have a config.php with just one variable and it's easy enough to update in each of the four files whenever the min PHP version changes. I was just kind of throwing out ideas.

[sreichel]

I can live w/o, but can we add styling to some css-files? 😎

[fballiano]

In my opinion I would remove that message, we don’t check php extensions or any other thing, only php version.. I don’t see its utility this much as to have it in every call.

[justinbeaty]

I wouldn't bother to add any CSS to the message, literally nobody should ever see it.

I also don't care if we remove it, I already do that in my local fork. But perhaps more people should put in their opinion on removing it.

[justinbeaty]

Converting to draft since I think (if we choose to keep it) that we can put it into bootstrap.php. Maybe also check for PHP modules there, which could be useful now that IIRC we require intl.

[fballiano]

As we were saying in another pr we could have a system check page instead of checking at every request

[justinbeaty]

As we were saying in another pr we could have a system check page instead of checking at every request

Yes, but if the error is an incompatible PHP version or a missing module, it’s possible the user may not be able to reach that page.

Anyway, I will be off until tomorrow morning. Will modify this PR then, and then it’s up to the community/maintainers on what direction to go.

[fballiano]

It could be memory setting or many other parameters. Some projects have a system check script to call on cli

[sreichel]

As we were saying in another pr we could have a system check page instead of checking at every request

N98 magerun provides sys:check command ... head over here netz98/n98-magerun#1275 and lets finish this.

[luigifab]

I suggest to move it to Mage.php (like my root check even you are unhappy), or to drop it, :).
When OpenMage is installed with composer, composer check PHP version at every request.

[elidrissidev]

install.php is meant to be invoked from the cli, I think that's why there wasn't HTML before.

[justinbeaty]

When OpenMage is installed with composer, composer check PHP version at every request.

@luigifab is right. As long as you don't install with --ignore-platform-reqs (although --ignore-platform-req=ext-* is okay) then the file vendor/composer/platform_check.php checks PHP versions in the autoloader.

Since the release builder has the composer autoloader, then all new releases will check PHP version already. So I would vote to close this PR and open another to simply remove these lines from the root files.

Although one small note: platform_check.php wants >= 7.4.0. This is because: OpenMage requires magento-composer-installer, which requires symfony/console, which requires symfony/service-contracts, which requires psr/container, which requires "php": ">=7.4.0".

So we should bump the min required version, or remove magento-composer-installer in the built files which is something I tried to do here: https://github.com/fballiano/openmage/pull/3

[fballiano]

let's remove these lines and bump to 7.4, i'm fine with those