Fix comparison of PHP versions #25138
Conversation
Use the builtin function `version_compare` to check an app's compatibility with the available PHP version, instead of reusing the `OC\App\CompareVersion::isCompatible` method which is intended to compare Nextcloud versions. PHP version strings do not always necessarily follow the simple Major.Minor.Patch format used by Nextcloud and therefore cannot be properly compared by that method. Signed-off-by: Damien Goutte-Gattat <dgouttegattat@incenp.org>
|
/backport to stable20 |
| @@ -113,12 +113,12 @@ protected function fetch($ETag, $content, $allowUnstable = false) { | |||
| $phpVersion = $versionParser->getVersion($release['rawPhpVersionSpec']); | |||
| $minPhpVersion = $phpVersion->getMinimumVersion(); | |||
| $maxPhpVersion = $phpVersion->getMaximumVersion(); | |||
| $minPhpFulfilled = $minPhpVersion === '' || $this->compareVersion->isCompatible( | |||
| $minPhpFulfilled = $minPhpVersion === '' || version_compare( | |||
There was a problem hiding this comment.
Could we use OC\App\DependencyAnalyzer instead?
There was a problem hiding this comment.
I just had a look at that class and especially its analyzePhpVersion method. It seems to me that using it would be an over-complicated way of doing what version_compare already does…
Maybe using OC\App\DependencyAnalyzer would be better for consistency with what is done in other parts of Nextcloud, but in that case I would have to leave that to Nextcloud developers who know the code base better than I do. I am simply proposing a no-brainer patch to fix rapidly what is in my opinion a pretty serious issue, as it makes a whole range of apps impossible to install — including security-related apps such as twofactor_totp and twofactor_u2f.
There was a problem hiding this comment.
Yeah, sorry didn't check the author. Let's bring in @ChristophWurst
|
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
|
I think that patch should also be applied to line 124 where the same function is used. My NC 20.0.5 just blew another GB log. |
|
@JohnnyHagen could you please open a PR? |
|
I'm too late, it's already there: https://github.com/nextcloud/server/blob/master/lib/private/App/AppStore/Fetcher/AppFetcher.php, line 121 (not 124). |
|
I think we might have to revert and redo this fix. See nextcloud/mail#4383 (comment) for context. Basically php's builtin version compare is not always giving expected results. If an apps says it supports up to 7.4 then 7.4.14 meets this requirement, but version_compare says no. |
|
So |
|
Well that or we just make the regex |
|
Issue still exists in 20.0.6, e.g. leading to |
|
It's not "still exists", but "was introduced with" |
Use the builtin function
version_compareto check an app'scompatibility with the available PHP version, instead of reusing
the
OC\App\CompareVersion::isCompatiblemethod which is intendedto compare Nextcloud versions. PHP version strings do not always
necessarily follow the simple Major.Minor.Patch format used by
Nextcloud and therefore cannot be properly compared by that method.
This tentatively fixes issue #25137.