@felixarntz What do you think about this including a change:

--- a/package.json
+++ b/package.json
@@ -31,7 +31,6 @@
  },
  "main": "blocks/index.js",
  "scripts": {
+  "install": "composer install",
   "build": "grunt build && grunt create-build-zip",
   "deploy": "grunt deploy",
   "dev": "cross-env BABEL_ENV=default webpack --watch"

Would this be a good idea or would it be trying to do too much? It's not really so hard to do npm install && composer install vs just npm install. Also, arguments may need to be passed like composer install --prefer-source which wouldn't be possible if composer install were invoked automatically upon npm install. So I think I just answered my own question 😄

This depends on xwp/wp-dev-lib#278 which upon merging will make a 0.2 version available for us to reference. Update: That PR has been merged. Now waiting on tag.

Something else we should explore down the road is automatic installation of the pre-commit hook as described here: https://github.com/xwp/wp-dev-lib/#configure-the-git-pre-commit-hook

A problem is that Lando isn't playing nice with pre-commit so perhaps we should hold off on that for now: lando/lando#904

@westonruter
Agreed with your own response, I think it's usually most flexible to not intertwine composer.json commands or shortcuts with package.json commands or shortcuts. In the case pointed out, I'd definitely say it's not worth it.

Regarding the pre-commit hook, I think we should move forward with this now, as it shouldn't rely on Lando anyway. IMO such a hook should not be tied to a certain environment, it should only rely on the tools that are installed from the repo itself. In that regard, we should also consider installing PHPCS and PHPUnit ourselves. Note that even with Lando, a pre commit hook relying on the tools directly could still be run either from inside the container or outside of it.

I didn't yet review, but definitely approve of this change. Is it ready for review yet? Cause there are still a few boxes to be checked in the description.

In that regard, we should also consider installing PHPCS and PHPUnit ourselves. Note that even with Lando, a pre commit hook relying on the tools directly could still be run either from inside the container or outside of it.

We are actually installing phpcs and phpunit ourselves via Composer, but the problem is invoking them inside of Lando from the host machine. For phpcs this isn't a problem really since it's likely that PHP is installed on the host, but for phpunit there has to be the database connection and WordPress environment which is inside of Lando. And running lando phpunit is not working inside of the pre-commit hook.

I didn't yet review, but definitely approve of this change. Is it ready for review yet? Cause there are still a few boxes to be checked in the description.

Yes, it is ready. I've updated the todos based on your confirmation of composer install.

We are actually installing phpcs and phpunit ourselves via Composer

My mistake. Actually we aren't currently installing phpunit and that was due to compatibility with PHP 5.3 I believe, which is no longer a problem.

Installing phpunit via Composer I believe is still a problem because one version is needed by PHP 5.x but another by PHP 7, which is why Gutenberg also does not install PHPUnit via composer.json but does so imperatively in:

https://github.com/WordPress/gutenberg/blob/480e47742b419e8cddbcc49556b1783c14da7051/bin/install-php-phpunit.sh#L21-L26

My mistake. Actually we aren't currently installing phpunit and that was due to compatibility with PHP 5.3 I believe, which is no longer a problem.

Yeah, wp-dev-lib installs a version of phpunit depending on the version of PHP running on Travis. However, it would be better to fix that for consistent tests both locally and on Travis.

Using phpunit 4+ will also require removing the Composer platform lock to PHP 5.4 or else it will refuse to install anything newer than phpunit 4:

one version is needed by PHP 5.x but another by PHP 7, which is why Gutenberg also does not install PHPUnit via composer.json but does so imperatively

@westonruter Could we run PHP unit tests only in PHP 7. Are there tests exclusive to older versions of PHP?

Could we run PHP unit tests only in PHP 7. Are there tests exclusive to older versions of PHP?

@kasparsd Perhaps, but not running tests in PHP<7 would be dangerous because some issues related to specific PHP versions only are exposed when running PHPUnit tests on those PHP versions.

@westonruter Would https://github.com/PHPCompatibility/PHPCompatibility take care of all the PHP version-specific issues?

Core is running tests with a different version of phpunit based on the version of PHP:

https://github.com/WordPress/wordpress-develop/blob/a0978e8222e09acaf41788cbd4eadae3b60879bc/.travis.yml#L66-L87

I'm having trouble thinking of a unit test that would fail with a different version of phpunit and not be related to PHP syntax/feature differences.

Would https://github.com/PHPCompatibility/PHPCompatibility take care of all the PHP version-specific issues?

It would catch a lot but it wouldn't catch everything.

I'm having trouble thinking of a unit test that would fail with a different version of phpunit and not be related to PHP syntax/feature differences.

The different version of phpunit isn't the problem. The problem is the different versions of PHP. There is code that targets specific PHP versions (which come with different library versions):

This latter case is irrelevant now given the upgrade to PHP 5.4, but still. Note the phpcs:ignore for PHPCompatibility because we're explicitly needing to use code that is for an older version. So for other cases like this, the PHPCompatibility tests wouldn't help make sure that the code actually works in the older PHP versions.

And running lando phpunit is not working inside of the pre-commit hook.

I would strongly advise to not make this project depend on Lando in any way, for the same reason I disagree on #1825. IMO a pre-commit hook should apply to the system that it is run from: It should just work after running npm install and composer install. In other words, the pre-commit hook should call vendor/bin/phpunit (as installed from Composer, more on that below) instead of lando phpunit.

Installing phpunit via Composer I believe is still a problem because one version is needed by PHP 5.x but another by PHP 7

We should still install these dependencies via Composer. For CI, this can easily be worked around by putting specific clauses in place for the older versions. In terms of workload and custom tweaks, this is similar to the additional work that needs to be made to install PHPUnit manually in every CI process, but it has the benefit that the default setup on your dev environment is at least proper and not relying on something from the outside that you might not have installed. I've used the approach of installing PHPUnit from Composer in multiple plugins, even those going as far back as supporting PHP 5.2, and there's no issue when working around this in .travis.yml to support the very old versions.

Yeah, wp-dev-lib installs a version of phpunit depending on the version of PHP running on Travis. However, it would be better to fix that for consistent tests both locally and on Travis.

I wasn't aware of that. That dependency installing PHPUnit is perfectly fine with me, we should ensure to use it though (again, except for the Travis-CI builds which don't even support Composer). The 4+ is the correct version constraint there.

To summarize these ideas, we should use PHPUnit via Composer everywhere, except in some CI environments, where something like this in .travis.yml works great:

  - |
    case "$TRAVIS_PHP_VERSION" in
      5.6|5.5|5.4|5.3)
        composer global require "phpunit/phpunit:^4"
        composer install
        PHPUNIT_BIN="phpunit"
        ;;
      5.2)
        PHPUNIT_BIN="phpunit"
        ;;
      *)
        composer install
        PHPUNIT_BIN="vendor/bin/phpunit"
        ;;
    esac

Then, the file later calls $PHPUNIT_BIN (with args as necessary). The snippet here even works for 5.2 and 5.3, which we obviously wouldn't need (particularly great not to support 5.2 since that doesn't even support Composer).

Btw I completely agree with PHPCompatibility not being sufficient to discover issues with old versions. PHPUnit tests need to run for every PHP version we support.

Using phpunit 4+ will also require removing the Composer platform lock to PHP 5.4 or else it will refuse to install anything newer than phpunit 4:

That's a good point, I didn't think about it when I introduced that lock. We should probably fix that. It would be perfect to have the possibility to only set the fixed platform to 5.4 when doing the build. I've looked into this for about an hour now, but there seems to be no proper support for this. I'll look into it in the near future again, for now let's keep config.platform to 5.4, even if that means we always run an old PHPUnit version, even locally.

We should still install these dependencies via Composer.

Could we do something like:

"require-dev": {
	"phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0"
},

per https://getcomposer.org/doc/articles/versions.md#version-range

It should then pickup the version supported by the version of PHP available on the system.

I would strongly advise to not make this project depend on Lando in any way, for the same reason I disagree on #1825.

Actually I didn't intend necessarily that the project would depend on Lando. I meant more generally that invoking lando in pre-commit seems broken in Lando right now. So for example, on my host machine I have aliases like:

alias wp="lando wp"
alias phpunit="lando phpunit"
# ...

But yes, the pre-commit hook should be calling vendor/bin/phpunit as you say:

In other words, the pre-commit hook should call vendor/bin/phpunit (as installed from Composer, more on that below) instead of lando phpunit.

Nevertheless, the pre-commit hook is not strictly part of the repo, so having variations based on the user's own development environment doesn't seem in poor form. Actually, the pre-commit hook in wp-dev-lib has built-in support to run PHPUnit tests inside of VVV when the pre-commit hook is being invoked from the host machine:

https://github.com/xwp/wp-dev-lib/blob/a458f7915c672e0d6431499040d6d8cd135a532a/scripts/check-diff.sh#L630-L689

I forgot that there is actually some Docker support in there as well.

Could we do something like:

"require-dev": {
	"phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0"
},

per https://getcomposer.org/doc/articles/versions.md#version-range

@kasparsd That looks promising! The only thing is that the config.platform would need to be removed, yes?

The only thing is that the config.platform would need to be removed, yes?

@westonruter Yes, correct.

@westonruter Maybe leave the phpunit update and the pre-commit autoinstall to a different PR?

That looks promising! The only thing is that the config.platform would need to be removed, yes?

Regarding the PHPUnit version, it could also use >=4.0, which would be a bit simpler to read. However, we shouldn't remove config.platform until we've found a better replacement for that IMO. Alternatively, if we remove it, we need to ensure to always have all entries in require use fixed versions (no ranges of any kind) that we know to work with PHP 5.4. The require-dev versions could use ranges since those aren't included in a plugin build anyway.

Maybe leave the phpunit update and the pre-commit autoinstall to a different PR?

Probably even two PRs :)