Downgrade to PHP 7.4, move from Pest to PHPUnit, establish CI

[reimic]

What is this PR doing?

Acts as an intermediary step in reaching goals set in #26 , #27.

• Downgrades the lib to PHP 7.4
• Adds 'Check PHP CS == 7.4' step to GitHub Actions
• Replaces Pest with PHPUnit
• Rewrites RmStep tests in PHPUnit
• Adds matrix unit tests step for all operating systems and all PHP versions >= 7.4

Caveats

• Dependencies versions are wildcards. This is inherently unstable, but our true goal is to have stable 7.0 support. Establishing fixed versions is a tedious job that does not seem crucial at this temporary step.
• Unit tests should include prefer-lowest as one of the arguments in the dependency_version list. This will allow for the broadest version support. But at this point in time it fails at 7.4. Fixing this serves little purpose as, see logic above.
• Although we know PHPUnit 6.5 should be used, since this is the newest versions supporting PHP == 7.0, PHPUnit 9.6 is used here. This is due to all lower versions throwing the pesky PHP Fatal error: Cannot acquire reference to $GLOBALS. This is still a step in the right direction as writing PHPUnit tests in 9.6 and then downgrading them to 6.5 is still easier, than writing in Pest and then going for PHPUnit 6.5.
• It should be analyzed if it is in fact beneficial to run the matrix with each pull/pr. Alternatives: cron, only pr and more.

[reimic]

Coding standards matching WordPress' support for 7.0:

• downgrade the whole lib to 7.0,
• implement a GitHub actions step checking for 7.0 coding standards:
  • this is surprisingly not straight forward, since many quality tools require a greater PHP version,
  • older version exist, but why use a tool in a version that is no longer supported, when supported tools that handle PHP 7.0 exist?
  • PHP CS Fixer requires >=7.4, source
  • Laravel Pint is build on PHP CS Fixer, source
  • PHPStan requires >=7.2, source
  • CodeSniffer seems ok, also is integrated into PHPStorm source
  • Mess Detector also seems ok, also is integrated into PHPStorm source

I'll explore CodeSniffer and Mess Detector more. Any takes @adamziel ?

[adamziel]

downgrade the whole lib to 7.0,

There's a lot happening here. Let's ship this one with failing tests and downgrade in a separate PR.

this is surprisingly not straight forward, since many quality tools require a greater PHP version,

That's fine. People should be able to run the published Blueprints package in their PHP 7 codebase, but contributing to the project doesn't have the same constraint. It's perfectly fine for the developer tools to require PHP 8. Test runner is more tricky since we want the tests to pass on PHP 7 so, presumably, the runner should be compatible with that version. If that means we need to move from Pest to PHPUnit, then let's do that.

CodeSniffer seems ok, also is integrated into PHPStorm source

This is the standard WordPress tool, let's go with that one.

[adamziel]

I wonder if composer can even enforce PHP 7.0 in the project while allowing the dev tools to run on PHP 8.2 🤔 Perhaps we'll need a separate list of global composer dependencies just to free them from these restrictions.

[reimic]

If that means we need to move from Pest to PHPUnit, then let's do that.

It does. I'll start with my recent pr. For PHP 7.0 the valid PHPUnit version is 6.5. source Of course it is no longer supported. But this is expected.

What is more, this would be beneficial for cross-version and cross-os testing, with something along these lines:

tests:  
    runs-on: ${{ matrix.os }}  
    strategy:  
      fail-fast: true  
      matrix:  
        os: [ubuntu-latest, macos-latest, windows-latest]  
        php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3']  
        dependency_version: [prefer-lowest, prefer-stable]  

I wonder if composer can even enforce PHP 7.0 in the project while allowing the dev tools to run on PHP 8.2

Not with a trivial solution. The question being - which tolls lack a acceptable 7.0 alternative?

[reimic]

It's quite confusing. For PHP 7.0 "symfony/filesystem":"^3.4" is the highest valid version. But it conflicts with "symfony/http-kernel" which in turn has to be downgraded to "symfony/http-kernel": "^3.4.49" which is incompatibile with a buch of stuff. Among others "symfony/http-client" which has no PHP 7.0 compabile version at all.

For now I'll do a workaround and focus on moving from Pest to PHPUnit. But those will use Filesystem in it's current version, which will be downloaded later. I guess this is the pinnacle of iterative development...

[reimic]

Let's continue in #30 and focus on a basic pipeline and Pest -> PHPUnits change.

[reimic]

Well. After lengthy discussions with @adamziel it became apparent that downgrading / testing are more interconnected than initially expected. See problems mentioned at #30.

So, to meet #27 and finally #26, we'll downgrade the code as far back as possible without extraneous effort. Essentially to PHP 7.4. At this version we can rewrite / keep writing unit tests in PHPUnit 6.5 and have pipelines run somewhat smoothly.

And after that is done, and the code mostly works 👼, we can carefully move to 7.0. Which will include replacing or transpiling dependencies.

[adamziel]

Actually, what is the specific issue with the word resource in PHP 7.0? I just tried running the following code and it worked just fine:

<?php

namespace Test\Resource;

$resource = 'test';

class Resource {}

function resource($resource) { echo "This is fine"; }

resource('a');

You can try it here: https://3v4l.org/nEQv0#v7.0.8

[adamziel]

This one is close. Let's conclude the discussion about the word Resource and resort back to using it if there are no issues with it, resolve the merge conflicts, and merge.

[reimic]

All done now. 😎

[adamziel]

@reimic This needs to be resolved before the PR can be merged:

[adamziel]

Let's use @TODO: for these to make them easier to find. Not a blocker here, just a note for the future.

Also, this one isn't very clear. Why does this class need an additional review? What to look for?

[reimic]

Ok, for the first part. Regarding the reason for marking it for review. It had a lot of commented out code, I wanted to signal that that could be addressed, but there was no reason to do that in this PR.

[adamziel]

Let's just add that extension to composer.json:

{
   "require": {
        "ext-json": "*"
   }
}

[adamziel]

I left a few nitpicks, but this PR looks pretty good otherwise. I'll go ahead and merge, let's address any remaining issues in a follow-up one.