My system has PHP 7.1.12 installed, but I was unable to enable a module on the UI that contains "php: 7.1" restriction in its info.yml file. After some debugging I have realized that the problem is with the logic how the current PHP version is compared with the minimum PHP version requirement.

bug

POC https://3v4l.org/X8OCV

It seems 8.5.x-dev is also affected.

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

Title: Incorrect usage of version_compare() prevents installation of module with minimum PHP version requirement » Unable to install module with minimum PHP version requirement on the UI
Status: Active » Needs review
StatusFileSize
new1.12 KB

Status: Needs review » Needs work

The last submitted patch, 2: drupal-module_install_problem_with_minimum_php_version_requirement-2934098-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mxr576’s picture

Fix failing automated test.

mxr576’s picture

Status: Needs work » Needs review
avpaderno’s picture

Status: Needs review » Needs work

On my Ubuntu 16.04.3 installation, with PHP 7.1.12-3+ubuntu16.04.1+deb.sury.org+1, this is what I get running the existing code and the code used from the patch from Bash.

echo version_compare(phpversion(), "7.1"); // 1
$minimum_php = substr("7.1", 0,3); echo version_compare(substr(phpversion(), 0, 3), $minimum_php); // 0

If then I print the PHP version required from the module (7.1) using the existing code and the patch code, this is what I get.

$required = "7.1" . (substr_count("7.1", ".") < 2 ? ".*" : ""); echo $required; // 7.1.*
$minimum_php = substr("7.1", 0,3); $php_required = $minimum_php . (substr_count($minimum_php, ".") < 2 ? ".*" : ""); echo $php_required"; // 7.1.*

I am getting the same result from the existing code, and the code used from the patch.

I find strange that 7.1 is getting printed as 7.1000000000000004.*. It seems 7.1 is getting changed in a floating point number more than an issue with how the existing code works.

mxr576’s picture

Status: Needs work » Needs review
StatusFileSize
new227.2 KB

Updated the POC with the PHP version that you have referred to, still getting different results: https://3v4l.org/9AOC0.
Sorry, coffee have not kicked yet. :)

I find strange that 7.1 is getting printed as 7.1000000000000004.*. It seems 7.1 is getting changed in a floating point number....

I can also agree with this part of your sentence, but I'd rather change the wording, use "parsed" instead of "printed" because this is what I could see in xDebug:

xdebug

mxr576’s picture

Quick hotfix to the problem... Adding php: "7.1" instead of php: 7.1 to a module's info.yml file. So could it be just a documentation related issue?

avpaderno’s picture

Yes, the YAML parser would take php: 7.1 as setting a floating point value.
It should be explicitly documented that the expected value is a string, but the code could also handle the php: 7.1 case.

mxr576’s picture

Status: Needs review » Needs work

Just make clear the next steps. Does it mean that the `php: 7.1` should not be parsed as `7.1000000000000004.` and this is what should be fixed?

avpaderno’s picture

This is either:

  • The documentation that assumes the YAML parser returns strings also for values like 7.1 (as happens with the code parsing the .info files in Drupal 7 and earlier versions)
  • The code that should be prepared to get a floating value for php: 7.1

It is probably the first case, but I think the code should be able to handle this case.

implode('.', explode('.', $module->info['php'])) works, but is there a quicker way?

mxr576’s picture

Version: 8.4.x-dev » 8.5.x-dev
mxr576’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mxr576’s picture

Version: 8.6.x-dev » 8.7.x-dev
Component: base system » extension system
Status: Needs work » Needs review
Issue tags: -PHP 7.0 (duplicate)
Related issues: +#3053832: Refactor ModuleListForm
StatusFileSize
new2.16 KB
new3.82 KB

Well, I have just bumped into this old issue again on a project.

I know that there are plans and open issues to refactor the extension system and the module listing page but this is a serious bug that should have been fixed earlier. This bug requires a smaller fix so it has a higher chance that it could be merged sooner than other huge patches in related issues.

Took the idea from the \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest::testModulesListFormWithInvalidInfoFile() to create the necessary test modules from code when the test is running instead of adding them to the codebase.

mxr576’s picture

Meh, test only patch should not pass and it does not pass in my local. Try to figure out why it passes on the CI.

mxr576’s picture

Ehm, probably found the reason why it passes on the CI: https://bugs.php.net/bug.php?id=75213

The problem only occurs if < 2.0.3 is installed from the YAML Pecl extension.

So the test-only patch passes because YAML PECL extension is either not installed on the CI and Drupal fallbacks to symfony/yaml or at least 2.0.3 is installed. But this issue still exists with older YAML PECL versions and I am not sure how it could be enforced to have >= 2.0.3 installed from this optional PHP extension. Consequently, probably the fix in this patch is the best option if we want to add a general fix to Drupal core.

tstoeckler’s picture

StatusFileSize
new962 bytes

Great research @mxr576! I found that PHP does actually allow you to fetch extension versions, so I think we can actually use that to only use the PECL YAML serializer if the version is recent enough. See the attached patch. Does that work for you?

mxr576’s picture

Works for me, but I would probably also keep the new test case that I added which ensures this problem never happens again in the future.

how it could be enforced to have >= 2.0.3 installed from this optional PHP extension.

What I meant with this is we can not enforce the PHP extension version from composer.json. :)

tstoeckler’s picture

Yes, keeping the test coverage definitely makes sense. My patch was just meant as an idea for an alternate approach.

Status: Needs review » Needs work

The last submitted patch, 18: 2934098-18.patch, failed testing. View results

mxr576’s picture

Assigned: Unassigned » mxr576

Lemme check what has failed and why.

mxr576’s picture

index 69a7ed74ea..3773a73897 100644
--- a/core/modules/config/tests/src/Functional/ConfigSingleImportExportTest.php
+++ b/core/modules/config/tests/src/Functional/ConfigSingleImportExportTest.php
@@ -172,7 +172,8 @@ public function testImport() {
       'import' => $import,
     ];
     $this->drupalPostForm('admin/config/development/configuration/single/import', $edit, t('Import'));
-    if (extension_loaded('yaml')) {
+    // @see \Drupal\Component\Serialization\Yaml::getSerializer()
+    if (extension_loaded('yaml') && version_compare(phpversion('yaml'), '2.0.3', '>=')) {
       // If the yaml extension is loaded it will work but not create the PHP
       // object.

Well, I could not find a better than this to fix the failing test. Even if there is a serialization.yaml service, everywhere that static methods are being called from the YAML class ¯\_(ツ)_/¯. Otherwise, I could decorate the service and expose the actual serialization for the test to modify the original condition to something like "if Symfony serializer is being used underneath..., otherwise". (Whey there are services in Drupal core that referring to classes which only has public static methods? Like Settings... o.O)

Fixed the failing test and added my new test case from #15 to fix in #18.

tstoeckler’s picture

Yes, I think that is the correct way to fix that test, thanks for figuring that out! Patch looks good to me, but couldn't hurt for another review of the added test coverage, didn't review that in depth yet, so not going to RTBC yet.

andypost’s picture

Version: 8.7.x-dev » 8.8.x-dev
Status: Needs review » Reviewed & tested by the community

It looks good to go

badrange’s picture

This is the D8 major issue that has been RTBC'd the longest. Would be great if it got merged before it gets outdated and requires a reroll.. :-)

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

The patch has test fails on php 7.3 it seems

mxr576’s picture

Assigned: Unassigned » mxr576

Weird, I'll look into this,hopefully this is not caused by another extension issue on PHP 7.3 😜

mxr576’s picture

Assigned: mxr576 » Unassigned

Okay, I may need some help to figure out why it is failing on the CI...

PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.8 with Xdebug 2.7.2
Configuration: /var/www/html/core/phpunit.xml

Testing Drupal\Tests\system\Functional\Module\DependencyTest
Test 'Drupal\Tests\system\Functional\Module\DependencyTest::testCompatiblePhpVersionDependency' started
Test 'Drupal\Tests\system\Functional\Module\DependencyTest::testCompatiblePhpVersionDependency' ended


Time: 12.1 seconds, Memory: 6.00MB

OK (1 test, 13 assertions)
➜  drupal git:(8.7.x) ✗ docker-compose exec php php -r 'foreach (get_loaded_extensions() as $extension) echo $extension . " " . phpversion($extension) . PHP_EOL;' | grep yaml
yaml 2.0.4
 $ composer show symfony/yaml | grep version
versions : * v3.4.30

(which is the same version as the CI installed as I can see)

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Needs review
Issue tags: -version_compare, -module install problem +Bug Smash Initiative

From #17. This was fixed in yaml-2.0.3 which was released on 2018-11-13, the latest version is yaml-2.2.2 released on 2021-10-24. Does it still make sense to add testing?

quietone’s picture

Status: Needs review » Closed (cannot reproduce)

I did some manual testing of this on Drupal 9.4.x, standard install, and a local test module. The site is using PHP 8.0.7. I set the php property in the test module to 8.0.1. 8.0, 8.0.8 and 8.1. In all cases the results were correct.

Therefore, closing as cannot reproduce. If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core").

Thanks!