Keep module compatible with nikic/php-parser v4, while adding v5 support

Only the https://git.drupalcode.org/project/upgrade_status/-/blob/4.x/src/ThemeFu... uses the PHP parser I think directly. It would need to be attempted whether there were any API changes related to those uses.

Interestingly it only fails on Drupal 9 with

1) Drupal\Tests\upgrade_status\Functional\UpgradeStatusAnalyzeTest::testAnalyzer
Failed asserting that 1 matches expected 3.
/builds/project/upgrade_status/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/project/upgrade_status/tests/src/Functional/UpgradeStatusAnalyzeTest.php:197
/builds/project/upgrade_status/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

This is where this fails, so clearly does not do the theme function checking properly:

    $report = $key_value->get('upgrade_status_test_theme_functions');
    $this->assertNotEmpty($report);
    if ($this->getDrupalCoreMajorVersion() < 10) {
      $this->assertEquals(3, $report['data']['totals']['file_errors']);

Help is welcome to figure out how to make it compatible :)

Keeping v4 did not help, Drupal 9 would also take v5 and still fail there. It does not fail in Drupal 10 because the code it would affect does not actually run, so it does not help. Would be nice if composer would have optional dependencies based on dependency criteria haha. Since we don't seem to need it in the Drupal 10 to 11 path. Anyway, until then the solution is to make it work :) Help still welcome.

Thanks for the patch @Gábor Hojtsy!

I am not sure how to check if this fixes the Composer block ... After cloning and applying the patch, I tried running composer install inside the module, but nothing happened ... so I ended up installing the required external projects manually:

ddev composer require "mglaman/phpstan-drupal:^1.0.0"
ddev composer require "phpstan/phpstan-deprecation-rules:^1.0.0"
ddev composer require "dekor/php-array-table:^2.0"
ddev composer require "nikic/php-parser:^4.0.0|^5.0.0"
ddev composer require "webflo/drupal-finder:^1.2"
ddev composer require "symfony/process:^3.4|^4.0|^5.0|^6.0"

Most importantly, the nikic/php-parser parser was installed using those constraints:

$ ddev composer require "nikic/php-parser:^4.0.0|^5.0.0"
./composer.json has been updated
Running composer update nikic/php-parser
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 1 update, 0 removals
  - Upgrading nikic/php-parser (v4.18.0 => v5.0.2)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Upgrading nikic/php-parser (v4.18.0 => v5.0.2): Extracting archive
[...]

After these steps, I get the expected warnings on the report page:

	Project	Type	Status	Local version	Local 10-ready	Local scan result	Drupal.org version	Drupal.org 10-ready	Drupal.org issues	Plan
	Views Reference Field (viewsreference)	Contributed module	Installed	8.x-1.8	Incompatible	1 problem	Not applicable	Unchecked	Issues	N/A

I think this is a critical bug for the Upgrade Status module, since it's currently not possible to install in Drupal 9 with Composer, you need to git clone and install libraries with Composer as seen above (or maybe there is another method?) so changing the Status.

I created #3426870: Add test: Can Upgrade Status be downloaded with Composer?.

@ressa: but Drupal 9 does not require nikic/php-parser v5.0.0? This definitely cannot be committed as-is as shown by the test fails, it does not find theme functions anymore. There are likely changes in the parser in v5 that need to be accounted for in our uses of its classes/interfaces.

Sorry @Gábor Hojtsy, I was wrong.

I think what happened was that I tried to download the Upgrade Status module with Composer having Drush installed, and got this error:

$ ddev composer require drupal/upgrade_status
[...]
Your requirements could not be resolved to an installable set of packages.
Problem 1
  - drupal/upgrade_status[1.0.0, ..., 1.4.0, 2.0.0] require drupal/core ^8.6 -> found drupal/core[8.6.0, ..., 8.9.20] but the package is fixed to 9.5.11 (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.
  - drupal/upgrade_status[2.2.0, ..., 2.5.0] require drupal/core ^8.7 -> found drupal/core[8.7.0, ..., 8.9.20] but the package is fixed to 9.5.11 (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.
  - drupal/upgrade_status[2.6.0, ..., 2.9.0, 3.5.0, ..., 3.19.0, 4.0.0, ..., 4.1.0] require nikic/php-parser ^4.0.0 -> found nikic/php-parser[v4.0.0, ..., v4.18.0] but the package is fixed to v5.0.2 (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.
[...]

Since nikic/php-parser was the only non-Drupal project mentioned, I searched for that, and found this issue. Also, I am pretty sure I tried removing Drush, tried again, and failed ...

But I now read the tip on the project page about removing Drush. So I tried again just now with a fresh install with Drush installed, and failed (as expected) to confirm the problem. After removing Drush, the module installed without a problem, and works well. Sorry about the noise!

To make it more likely people find the hint on the project page when they search for help, perhaps nikic/php-parser could be mentioned, if that's always the problem?

If this fails with Your requirements could not be resolved... and nikic/php-parser ... the package is fixed to v5.0.2 [...]

Alternatively, and maybe better: Since you get this error in a standard fresh Drupal 9 installation with Drush installed, I suppose the majority run into this problem, right? Wouldn't it then be better, if the project page simply said this?

Install Upgrade Status on your existing Drupal site.

Due to compatibility issues, it is recommended to uninstall Drush with composer remove drush/drush before downloading Upgrade Status. You can add Drush again afterwards.
[...]

Ok I started looking into this a bit more. First the code has this comment

 * @todo Remove once Drupal 8 to 9 deprecations are not a focus anymore.
 *   This is not dependent on Drupal 8 core itself though, so we can keep
 *   it in Drupal 9 to 10 for the sake of exposing extremely outdated code.

So it could be mostly safely removed in a version that works only with Drupal 10 and 11. However since theme functions were deprecated in Drupal 8 and only removed in Drupal 10, the code compatible with Drupal 9 should still work. And that may still be installed with v5 of the parser, hm.

The theme function deprecation analyzer would not work on Drupal 10 anyway, due to the signature of the theme registry being different than how it uses it. But we don't run it on Drupal 10. :)

Ok both the compatibility layer with and without v4 is green. That seems to be due to PHP 7.4+ being tested which all take v5 anyway. PHP 7.4 is the only version supported by Drupal 9 anyway, so I think that's a safe assumption for now. Hosts have been removing older PHP versions more aggressively recently. Given that though the version without the compatibility layer is better IMHO.

Ok I also tried allowing PHP 7.3 to trigger the v4 of the parser from composer, but we also require Drush 11 which requires PHP 7.4+ so it is already not possible to install Upgrade Status on PHP 7.3 if you also use Drush. It would be quite some more work to go back and support Drush 10 again so we can keep Nikic v4 support. That sounds like a very theoretical scenario. So I think its fine to merge the v5 only update and accept that would be the version people go with. If there are reports of needing to go back and keep supporting v4 somehow, we can look into that then, but based on all I found here I don't expect that to happen.

Nope not that simple. On another fresh setup this blocks installing Upgrade Status now because PHPUnit 9 depends on Nikic parser v4 :(

https://packagist.org/packages/phpunit/phpunit#9.6.x-dev requires https://packagist.org/packages/phpunit/php-code-coverage#9.2.28 which has a nikic/php-parser: ^4.15 dependency.

I have no idea how does the standalone version pass the phpunit tests, although I also run it and seen it with my own eyes. But looks like we'll need to keep v4 compatibility for phpunit's sake.

Made some adjustments on the compatibility MR to be on top of what we have now.

Thanks @Gábor Hojtsy, it's now possible to download the module, even WITH Drush already installed:

composer require drupal/upgrade_status:4.x-dev@dev

But if I install the dev-tools with Drush present, I get this (it completes if I remove Drush):

$ ddev composer require --dev drupal/core-dev:9.5.11 --update-with-all-dependencies
./composer.json has been updated
Running composer update drupal/core-dev --with-all-dependencies
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.
  Problem 1
    - Root composer.json requires drupal/core-dev 9.5.11 -> satisfiable by drupal/core-dev[9.5.11].
    - drupal/core-dev 9.5.11 requires symfony/filesystem ^4.4 -> found symfony/filesystem[v4.4.0, ..., v4.4.42] but these were not loaded, likely because it conflicts with another require.

That's probably harder to resolve ...

Since you get this error in a standard fresh Drupal 9 installation with Drush installed, and the majority probably run into this problem, is this rewording worth considering?

Install Upgrade Status on your existing Drupal site.

Due to compatibility issues, it is recommended to uninstall Drush with composer remove drush/drush before downloading Upgrade Status. You can add Drush again afterwards.

[...]

This can then be removed:

If this fails with Your requirements could not be resolved..., you may need to remove Drush with composer remove drush/drush and add it back in after the developer dependencies as documented.

Updated to this

Installation

Due to third party PHP library dependencies, the module needs to be installed with Composer.

1. It is recommended to remove Drush before continuing to avoid common incompatibilities with dependencies. You'll add back Drush again at the end.

   $ composer remove drush/drush

2. To avoid false errors with scanned test code, you also need Drupal's developer dependencies.
   

   $ composer show drupal/core | grep versions
   $ composer require --dev drupal/core-dev:[copy version above] --update-with-all-dependencies
   

3. Add Upgrade Status itself to the codebase.

   $ composer require drupal/upgrade_status

4. If you removed Drush earlier, add it back now.

   $ composer require drush/drush

5. Finally, install the module using the Extend page or with Drush as you would any other module.

Thanks, it's much clearer now what needs to be done, and updaters will encounter less bumps in the road :)

Regularly, someone should check if dev-tools and Drush might eventually co-exist in peace, and then step 1 and 5 can be removed.

I am sorry I keep nagging about updating the steps. My only aim is make the upgrade process and future maintenance easier for non-experts.

Should we add this sentence, after the final step? After all, the majority of users will never need Drupal's developer dependencies ...

[...]
5. Finally, install the module using the Extend page or with Drush as you would any other module.

After you have completed the preparations for your upgrade to Drupal 10 from Drupal 9, uninstall and delete the Upgrade Status module and Drupal's developer dependencies (unless you need them) before actually upgrading to Drupal 10.

Upgrade Status and drupal-check compared #