Run PHPUnit jobs across multiple PHP versions - #46510

This ignores all .cache/.gitkeep files but /.cache/.gitkeep.
The /.cache/ folder is needed for the PHP Coding Standards CI job.

This deserves an explanatory comment inline as it's going to be really hard to find this PR comment when someone is trying to understand why the .cache directory is so zig-zagged here. Can we add the explanation, and include why a simpler formulation of the rules cannot provide what we need?

Is it not possible to have PHP Coding Standards use another directory? What is the reason for its use of .cache here anyway?

I also now see that we're telling git to keep the .cache directory, which seems surprising to me. why do we need to do this?

if this weren't .gitkeep'd then this surprising change to .gitignore presumably would also not be necessary

The reason for zig-zagging is the following:

It is not possible to re-include a file if a parent directory of that file is excluded. 

This deserves an explanatory comment inline as it's going to be really hard to find this PR comment when someone is trying to understand why the .cache directory is so zig-zagged here.

I agree. Fixed in 89a4d738e13c7d6ff51d54cfa2477eb0c713c67b

Currently, Gutenberg ignores all .cache folders (regardless of their location).
For compatibility reasons, these folders should still be ignored, while allowing the /.cache folder (in the root).
I haven't been able to come up with a simpler pattern that would allow that.

if this weren't .gitkeep'd then this surprising change to .gitignore presumably would also not be necessary

Git doesn't allow to commit empty folders. If it did, I would commit an empty folder.

Is it not possible to have PHP Coding Standards use another directory? What is the reason for its use of .cache here anyway?

Yes, it's possible, but I don't understand what's wrong with /.cache.

A similar Core job uses a folder with the same name - .cache.

sure but why do we need or want to commit an empty directory?

Because PHPCS expects the directory to which to write the cache file to exist. It will not create it.

So if the ruleset dictates that the cache file should be written to the .cache directory, that directory must exist as otherwise PHPCS will error out.
You could, of course, choose to just let the cache file be written to the system temp directory or to the project root...

Note: the PHPCS cache file itself should definitely be gitignored.

my suspicion was that it was something like this, but if that's the case, that doesn't explain why it needs to be in git, it merely notes that the directory needs to be present when running the linter.

if the entire purpose of coercing git to include an empty directory in the repo is so that a linter can run in CI then it would be simpler to create the directory when running the CI script with mkdir -p .cache or the equivalent.

Well, that depends. If the ruleset contains the cache directive, local CS runs will also expect the directory.

If the caching is only used in CI, I agree, a mkdir would suffice, but then again, why would the caching be limited to CI ?

Also note - in most projects which use a .cache directory for the PHPCS cache placement, it is generally because they want to cache those files in CI and Travis (yes, that's the completely outdated reason), didn't allow for caching individual files, only allowed for caching directories, which is why people resorted to having a .cache directory for this.

GH Actions, however, does allow for caching individual files, so I honestly don't know why that .cache directory is still used, especially for projects setting this up from scratch. (I can understand people not having revisited their previous reason for setting it up when converting from Travis to GHA, but that's not the case here)

Can we get an explanation of why we want to clear the cache once a week?
Generally time-based caches are useful in rare and somewhat emergency situations.

What is the reason we can't use a content-based cache?
Also do we know we need the cache? What do we expect it to provide for us?

This cache is both content and time-based.

hashFiles('**/composer.json', 'phpcs.xml.dist')

ensures that the cache key is content-based.

Also do we know we need the cache? What do we expect it to provide for us?

The main benefit of using the cache is to speed up the CI jobs.

What have you measured as the speedup in the CI jobs system?

In my tests,

phpcs -n --report-full 

takes about 12 seconds when the cache is disabled and 0 seconds (that's what GitHub shows) when the cache is enabled.
As the quantity of .php files in the project is growing, this difference will only increase over time.

hm. what if we introduced this first without the cache? in the grander scheme of all the test suites 12s is a drop in the bucket but the cache is going to likely create a non-trivial amount of headache at the point it causes problems in a PR.

I don't think these tests are going to be the slowest workflow suite, which means they aren't in the hot path, and if we merge first without the cache then if we think one is truly warranted then we can make a PR and gather a statistical measurement to determine if, when put into the system with 1700 open branches and lots of CI activity, if it still makes 12s of an improvement and if we think that's worth the additional complexity

Can we get an explanation of why we want to clear the cache once a week?
What is the reason we can't use a content-based cache?

Without having looked at the implementation in this particular workflow...

• Caches in GitHub Actions work differently than people often expect from other CI systems.
• A cache in GHA is created based on a key and NEVER updated.
• The only way to vacate the cache is to create a new cache with a new key.

So, for caches where the key doesn't change often, the cache tends to go more and more out of date over time. Adding a time based factor to the key helps to vacate the cache regularly.

Also see: ramsey/composer-install#234

hm. what if we introduced this first without the cache?

In case it would be decided to go that way, I would strongly recommend adding the --no-cache CLI argument to the PHPCS command in GHA.
As the cache directive is in the ruleset, PHPCS would otherwise needlessly create the cache file from scratch each time, which has a significant impact on the runtime of PHPCS due to the file-write operations.
As CLI args overrule the ruleset, adding no-cache should already speed up the CS run.

if this breaks in supported PHP versions, why not remove the dependency altogether? I'm worried about the disconnect between requiring this package and having this line in the workflow YML. best case scenario is that when this dependency is no longer required, we'll still be running this step, which is benign, but also is going to confuse someone why it's there.

is there a way to have composer tell us which packages are not compatible? or should we file a bug report with spatie/phpunit-watch to have that project list a minimum version if one is missing?

if this breaks in supported PHP versions, why not remove the dependency altogether?

This is because spatie/phpunit-watcher is required by the npm run test:php:watch command.
Thus it cannot be simply removed.
I was going to create a new GB issue to discuss the possibility of replacing it with a similar package that supports PHP < 7.2 (if that substitute package exists).

is there a way to have composer tell us which packages are not compatible?

Yes, composer already does that. It knows that this package is not compatible with PHP < 7.2 and refuses to install it if the current PHP version is lower than 7.2. So it should be removed before installing other packages (otherwise composer install fails).

or should we file a bug report with spatie/phpunit-watch to have that project list a minimum version if one is missing?

No, it's not missing. https://github.com/spatie/phpunit-watcher/blob/master/composer.json#L19

Yes, composer already does that. It knows that this package is not compatible with PHP < 7.2 and refuses to install it if the current PHP version is lower than 7.2. So it should be removed before installing other packages (otherwise composer install fails).

What I meant was that maybe we can avoid hard-coding a specific dependency as we've done here. It looks like if we spit out the tree format of composer show we can get that information, and then process it with a small script.

<?php

require 'vendor/autoload.php';

use Composer\Semver\Semver;

$PHP_VERSION = empty( getenv( 'PHP_VERSION' ) ) ? phpversion() : getenv( 'PHP_VERSION' );
$tree = json_decode( file_get_contents( 'php://stdin' ) );

function works_with_php_version( $version, $package ) {
	foreach ( $package->requires as $dep ) {
		if ( 'php' === $dep->name && ! Semver::satisfies( $version, $dep->version ) ) {
			//echo "Bailing on {$package->name} because of {$dep->version}" . PHP_EOL;
			return false;
		}

		if ( ! empty( $dep->requires ) && false === works_with_php_version( $version, $dep ) ) {
			return false;
		}
	}

	return true;
}

$unsupported = array();
foreach ( $tree->installed as $package ) {
	if ( ! works_with_php_version( $PHP_VERSION, $package ) ) {
		$unsupported[] = $package->name;
	}
}

echo implode( ',', $unsupported );

Invoked as composer show --tree --format=json | php php-supported.php this will spit out a list of packages that have dependencies which aren't supported on the running PHP version, or the version set in PHP_VERSION if such an ENV is set.

But it also includes another package not listed, and to the best of my ability to double-check it, it's right according to the listed minimum versions (when I set PHP_VERSION=5.6 and run it).

spatie/phpunit-watcher,yoast/phpunit-polyfills

While maybe someone registered the wrong minimum version, or we simply haven't hit the part that breaks yet, this is why I think it'd be good at least to explore determine this through inspecting the package definitions. The list will undoubtedly change more rapidly than we come around to update it.

Edit: I should have noted that Semver is also already pulled in to our composer deps.

Thanks for sharing the script.

we can avoid hard-coding a specific dependency as we've done here

Out of all installed Composer packages, this is the only package that gives trouble.
I didn't see the need to write a script for detecting such packages when I was working on the CI job, as this was the only "problematic" package.

If someone adds a composer package that is not compatible with PHP 5.6 or another Core-supported PHP version, I'd prefer that the author of that PR would be able to see the error (i.e., composer install should fail) instead of removing or hiding such packages with the help of a script.
Gutenberg should support the same PHP versions as Core.

If we had proper CI jobs before, spatie/phpunit-watcher wouldn't be merged to the repo (that's my assumption).
Removing spatie/phpunit-watcher before running PHPUnit tests is a workaround until that package is removed or replaced with another package that supports PHP 5.6.

yoast/phpunit-polyfills - I'm not sure I understand why it isn't compatible with PHP 5.6.
Please see this Packagist page:
https://packagist.org/packages/yoast/phpunit-polyfills

why it isn't compatible with PHP 5.6.

the dependencies of its dependencies state a minimum required version above 5.6. like I wrote above though, we may simply have not run into the places where it will break, or the package authors may have incorrectly listed their requirements.

if you run composer show --tree you will see where it comes in. ironically it's through phpunit so maybe we could safely eliminate that dependency chain as a given since we're already running.

If someone adds a composer package that is not compatible with PHP 5.6 or another Core-supported PHP version, I'd prefer that the author of that PR would be able to see the error (i.e., composer install should fail) instead of removing or hiding such packages with the help of a script.

I totally agree! that's why I asked about removing the package. do we know if anyone would even miss the npm run test:php:watch scripts? I wouldn't, because I don't use that due to the way it obscures the underlying test runner and won't let me do what I want to do with phpunit (and it doesn't even run when I try, immediately fails with an error).

it's not the worst thing to have this hard-coded exception in the code, but I'd love to do all we can to avoid it if possible. it makes me worry that this code will end up permanent in there and we'll lose our memory of why, and then when something is broken or needing maintenance it will make that work difficult, particularly if we end up removing the referenced package.

alternatively, the worst scenario I see which seems to be quite common is that we have all this special-casing hidden deep in our layers of build scripts and then something breaks and there's no linkage between the failure and the code in the build and testing scripts that leads someone to find the problem.

anyway that's more words than it warrants, but maybe we just ask if we can get rid of npm run test:php:watch and not have to deal with this.

@jrfnl thanks. I'm sure I'm wrong; I must be wrong. but I like being shown how and why I'm wrong, so if you think the script is wrong, well it's right there and you can point it out.

I'm clearly not an expert on this, but I am looking at the output of composer show --tree and for yoast/phpunit-polyfills it demonstrably lists dependencies for phpunit/phpunit which don't list 5.6 support.

as I noted above, it might make sense to special-case phpunit and omit checking its dependencies, given that we are already assuming the presence of phpunit, but what is wrong in the logic in the script?

I'm not just talking about the dependencies we currently have, but I'm wanting to make sure that when those dependencies change that we don't have to remember that this code exists, that it exists here, and remember to update it properly - not a good track record with relying that much on manual process.

yoast/phpunit-polyfills 1.0.4 Set of polyfills for changed PHPUnit functionality to allow for creating PHPUnit cross-version compatible tests
├──php >=5.4
└──phpunit/phpunit ^4.8.36 || ^5.7.21 || ^6.0 || ^7.0 || ^8.0 || ^9.0
   ├──doctrine/instantiator ^1.3.1
   │  └──php ^7.1 || ^8.0
   ├──ext-dom *
   ├──ext-json *
   ├──ext-libxml *
   ├──ext-mbstring *
   │  └──php >=7.1
   ├──ext-xml *
   ├──ext-xmlwriter *
   ├──myclabs/deep-copy ^1.10.1
   │  └──php ^7.1 || ^8.0

here's the truncated part of composer show --tree showing some of the dependencies not listing PHP 5.6 support. Maybe I'm overlooking that something is optional, or that there's a way to know if these will be supported. I'm definitely relying on package maintainers to list the correct support on their packages.

I'm sure I'm wrong; I must be wrong. but I like being shown how and why I'm wrong, so if you think the script is wrong, well it's right there and you can point it out.

@dmsnell Sorry, I hadn't looked at the script before - I just responded as the author of the PHPUnit Polyfills.

Looking at the script now, IMO, it is pretty clear where the error is.

1. The script will only work after a composer install has been run (as it needs vendor/autoload).
2. It will then analyse based on the dependencies which have been installed, which is highly dependent on which PHP version was used to run composer install (as there is no composer.lock file exactly for that reason: to allow for installing the correct PHPUnit version for each PHP version).

So... say I run composer install on PHP 8.0, it will install PHPUnit 9.x with the associated dependencies. Neither PHPUnit 9.x, nor any of its dependencies will be compatible with PHP 5.6.

No surprise then that the Polyfills will be flagged as incompatible, but that's not a problem with the polyfills, but a problem with a logic fallacy on which the script is based....

Sorry, I hadn't looked at the script before - I just responded as the author of the PHPUnit Polyfills.

I definitely wish I were smart enough to not have to read the questions I respond to 🙃

No surprise then that the Polyfills will be flagged as incompatible, but that's not a problem with the polyfills, but a problem with a logic fallacy on which the script is based....

If I understand what you're saying though it seems to imply the script is correct, the logic in the script is correct, and I simply made the mistake of running it after installing packages with a newer version of PHP, something which wouldn't happen in the CI run.

So I think in a way by trying to show that the script is flawed you actually confirmed it's the right logic, because my goal is to determine programmatically what needs to be removed based on the running script in a way that lets us avoid depending on people manually doing all the right steps.

However…

highly dependent on which PHP version was used to run composer install…So... say I run composer install on PHP 8.0, it will install PHPUnit 9.x with the associated dependencies. Neither PHPUnit 9.x, nor any of its dependencies will be compatible with PHP 5.6.

When I tried doing this I actually got the same result with PHP 5.6 where the composer show --tree output still declares yoast/phpunit-polyfills as unsupported, with identical output as listed above.

What I did was:

• clone the Gutenberg repo in a docker run php:5.6 environment
• install Composer through their script
• run rm composer.lock && rm -rf vendor && git reset --hard to make sure that nothing in vendor was stale
• run ./composer.phar install
• run ./composer.phar show --list

~$ php --version
PHP 5.6.40 (cli) (built: Jan 23 2019 00:04:26)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
~$ ./composer.phar --version
Composer version 2.2.18 2022-08-20 11:33:38

In this case though my script failed to run because of something in symphony/polyfill that depended on newer PHP versions. After manually removing spatie/phpunit-watcher it progressed further, but still failed to execute the autoloader due to an issue with unsupported strict_types inside the phpunit's Framework/Assets/Functions.php module.

Maybe my version of Composer is too new? To try and see if this could be the case I tried against with Composer 1.6 from the beginning of 2018 but its output is still the same, still showing phpunit/phpunit pulling in dependencies that rely on PHP 7.1 and newer.

So I guess the current state is that if we want to do this programmatically the script is telling us the information from the installed packages as it should, but we need to modify it to read the Semver dependencies directly instead of through the autoloader, since that fails on PHP 5.6, and also ensure that we're running it on the right set of installed packages.

require 'phar://composer.phar/vendor/composer/semver/src/Constraint/ConstraintInterface.php';
require 'phar://composer.phar/vendor/composer/semver/src/Constraint/MultiConstraint.php';
require 'phar://composer.phar/vendor/composer/semver/src/Constraint/Constraint.php';
require 'phar://composer.phar/vendor/composer/semver/src/Semver.php';
require 'phar://composer.phar/vendor/composer/semver/src/VersionParser.php';

use Composer\Semver\Semver;

$PHP_VERSION = empty( getenv( 'PHP_VERSION' ) ) ? phpversion() : getenv( 'PHP_VERSION' );
$tree = json_decode( file_get_contents( 'php://stdin' ) );

function works_with_php_version( $version, $package ) {
	foreach ( $package->requires as $dep ) {
		if ( 'php' === $dep->name && ! Semver::satisfies( $version, $dep->version ) ) {
			return false;
		}

		if ( ! empty( $dep->requires ) && false === works_with_php_version( $version, $dep ) ) {
			return false;
		}
	}

	return true;
}

$unsupported = array();
foreach ( $tree->installed as $package ) {
	if ( ! works_with_php_version( $PHP_VERSION, $package ) ) {
		$unsupported[] = $package->name;
	}
}

echo implode( ',', $unsupported );

I'd still like to better understand where I'm wrong here because I think I have tried doing what you told me I need to do but the results are the same as when I did it before.

If I understand what you're saying though it seems to imply the script is correct, the logic in the script is correct, and I simply made the mistake of running it after installing packages with a newer version of PHP, something which wouldn't happen in the CI run.

No, as, as you yourself just proved above.

• Your script is intended to find out which dependencies are incompatible so they can be removed before an install is run.
• Your script requires a Composer install to have been run already, which would fail when there are incompatible dependencies.

Maybe my version of Composer is too new?

For an install on PHP 5.6, you can use the latest of the Composer LTS 2.2.x series.
Composer 2.3.0 and higher require PHP 7.2 (or higher).

In this case though my script failed to run because of something in symphony/polyfill that depended on newer PHP versions. After manually removing spatie/phpunit-watcher it progressed further, but still failed to execute the autoloader due to an issue with unsupported strict_types inside the phpunit's Framework/Assets/Functions.php module.

I haven't dug in any deeper now, but I suspect this may have something to do with the default PHP version on your system not being PHP 5.6 and something somewhere along the way starting a new PHP process and using the system default PHP version instead of the PHP version you want it to use. Could that be it ?

So I guess the current state is that if we want to do this programmatically the script is telling us the information from the installed packages as it should, but we need to modify it to read the Semver dependencies directly instead of through the autoloader, since that fails on PHP 5.6, and also ensure that we're running it on the right set of installed packages.

Possibly, and while I appreciate you are enjoying the intellectual exercise of trying to understand this. @anton-vlasenko 's two liner in the GHA script fixes this with a lot less effort and higher accuracy.

Your script is intended to find out which dependencies are incompatible so they can be removed before an install is run.

that's not the case, but that's how I interpreted your statement when you said "which is highly dependent on which PHP version was used to run composer install". if the running PHP version determines what packages composer will install then I took that to imply it wouldn't install packages that are listed as incompatible.

For an install on PHP 5.6, you can use the latest of the Composer LTS 2.2.x series.

thanks. as indicated in my previous comment I did attempt this on 2.2 and 1.6 versions.

system default PHP version instead of the PHP version you want it to use. Could that be it ?

No I don't think so, because as indicated in my previous comment, PHP 5.6 was the only version of PHP installed within the Docker container. It was both the system default and the only available version to run.

Is all this necessary or mostly useful while developing this branch? Same question for the Docker debug info step below.

It's not necessary, but it's worthwhile.
E.g., it helps to confirm which version of Composer and/or PHP are used when installing Composer packages and running PHPUnit.

I'm asking because we don't currently do this for our other workflows, and it's a fair amount of noise, and some duplication (e.g. PHP and composer versions are already available through the matrix and through the PHP install step).

It's not problematic, per se, just curious why it's here as it seems like a step (to remove this debug logging) that might have been overlooked before prepping for final merge into trunk.

because we don't currently do this for our other workflows

We actually do this.

PHP and composer versions are already available through the matrix and through the PHP install step

Yes, they are through the matrix, but we cannot be sure that the CI jobs use these specific versions (until we log them).
Also, in the matrix, we only specify major and minor PHP versions (the same way it's implemented in Core).
The logging allows us to know the exact PHP version (including the point release) used when running particular CI jobs.

to know the exact PHP version (including the point release) used when running particular CI jobs.

That information is already available via the setup-php step.

Jonathan and me actually discussed removing a lot of this kind of debug noise from the WP Core workflows recently as the information is already available within the workflow.

That information is already available via the setup-php step.

I've removed logging Composer and PHP versions.
cb3ee089ee66b68af28305833e5868f7d517454d
2dfb8452e972553650ac309e497cb0fee252be15

Either specify it like so:

php: ['5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2']

or like so:

php:
  - '5.6'
  - '7.0'
  - '7.1'
  - '7.2'
  - '7.3'
  - '7.4'
  - '8.0'
  - '8.1'
  - '8.2'  

This is Yaml, not PHP.

I agree with @jrfnl on this, but…

This is Yaml, not PHP.

I don't see the reason to add snark in this comment. The code as written parses as expected and is valid YAML. We don't have to claim that @anton-vlasenko was unaware of which language is being written or put anyone down when their style is different than your style or even when we find stylistic differences in a project and a code submission.

Sorry if you interpreted my comment as snarky, definitely wasn't meant as such. It was just meant to convey that I understand where the inclination to write it like that comes from, nothing more.

I don't see @jrfnl 's comment as snarky either.
I sincerely appreciate your expertise and your code review, @jrfnl.
Thank you!