Build Tools: Install Composer dependencies as pre-lint step

[aduth]

Related:

• Automated Testing: Try composer non-interactive flag for Travis #21118
• https://wordpress.slack.com/archives/C02QB2JS7/p1586530859214100 (link requires registration)

This pull request seeks to install Composer dependencies as part of the PHP lint script. This is intended to improve the developer experience, where a default installation of the development environment would not allow for npm run lint-php to be run. This script assumes Composer dependencies to already have been installed. Unless the contributor explicitly installs these dependencies via the (undocumented) npm run wp-env run composer install command, they cannot run the tests locally.

These changes bring uniformity to how lint testing occurs between a contributor's environment and in Travis. Previously, our Travis environment was "privileged" to have these setup tasks run, but these setup tasks are not made available to standard contributors, nor are they documented.

Implementation Notes:

Very important to note: This changes Composer installation in Travis from using the (legacy?) "env" setup, to the new "wp-env" setup. It's most important in considering the potential impact of reintroducing the issue intended to have been addressed in #21118. Fortunately, it should be relatively obvious if there are any issues, since the Travis build would pass only if working correctly.

As illustrated by the above-referenced Slack discussion, I'm not particularly attached to this approach, but I do fear the developer experience is currently very lacking. It took me quite some time to figure out how to run PHP linting successfully. While this implementation can be "wasteful" in that it runs the composer install step even if dependencies are already installed, an alternative was not clear in how to reduce friction for contributors to run tests.

It should be relatively safe that this only applies to linting, since all of the Composer dependencies are specific to linting (source).

Possible alternatives could include:

• Put responsibility on the developer to install Composer dependencies as part of preparing their local environment, not much unlike an expectation that a developer should be running npm install before starting development work.
  • This must be clearly documented, and it is not currently.
• Something in the implementation of the wp-env scripts could infer based on an incoming command whether it should need to prepare the environment to be able to run the command.
• Add messaging to the failed test run if dependencies are absent, instructing the developer actions to take before trying again.
  • There is some prior art to this with the npm run test-php script:

Please ensure the WP_DEVELOP_DIR environment variable is set to your WordPress Development directory before running this script.

If you don't have a WordPress Development directory to use, run npm run env install to automatically configure one!

Testing Instructions:

1. Prepare a fresh development environment, following the "Getting Started" guide.
2. Run npm run lint-php.
3. Observe that it passes successfully.

[github-actions]

Size Change: 0 B

Total Size: 903 kB

ℹ️ View Unchanged

Filename	Size	Change
build/a11y/index.js	1.02 kB	0 B
build/annotations/index.js	3.62 kB	0 B
build/api-fetch/index.js	4.01 kB	0 B
build/autop/index.js	2.82 kB	0 B
build/blob/index.js	620 B	0 B
build/block-directory/index.js	6.24 kB	0 B
build/block-directory/style-rtl.css	760 B	0 B
build/block-directory/style.css	760 B	0 B
build/block-editor/index.js	104 kB	0 B
build/block-editor/style-rtl.css	10.2 kB	0 B
build/block-editor/style.css	10.2 kB	0 B
build/block-library/editor-rtl.css	7.22 kB	0 B
build/block-library/editor.css	7.22 kB	0 B
build/block-library/index.js	112 kB	0 B
build/block-library/style-rtl.css	7.15 kB	0 B
build/block-library/style.css	7.16 kB	0 B
build/block-library/theme-rtl.css	683 B	0 B
build/block-library/theme.css	685 B	0 B
build/block-serialization-default-parser/index.js	1.88 kB	0 B
build/block-serialization-spec-parser/index.js	3.1 kB	0 B
build/blocks/index.js	57.7 kB	0 B
build/components/index.js	198 kB	0 B
build/components/style-rtl.css	16.6 kB	0 B
build/components/style.css	16.5 kB	0 B
build/compose/index.js	6.66 kB	0 B
build/core-data/index.js	11.1 kB	0 B
build/data-controls/index.js	1.25 kB	0 B
build/data/index.js	8.43 kB	0 B
build/date/index.js	5.47 kB	0 B
build/deprecated/index.js	772 B	0 B
build/dom-ready/index.js	569 B	0 B
build/dom/index.js	3.1 kB	0 B
build/edit-navigation/index.js	3.1 kB	0 B
build/edit-navigation/style-rtl.css	279 B	0 B
build/edit-navigation/style.css	280 B	0 B
build/edit-post/index.js	93.5 kB	0 B
build/edit-post/style-rtl.css	12.3 kB	0 B
build/edit-post/style.css	12.3 kB	0 B
build/edit-site/index.js	10.4 kB	0 B
build/edit-site/style-rtl.css	5.02 kB	0 B
build/edit-site/style.css	5.02 kB	0 B
build/edit-widgets/index.js	7.53 kB	0 B
build/edit-widgets/style-rtl.css	3.74 kB	0 B
build/edit-widgets/style.css	3.73 kB	0 B
build/editor/editor-styles-rtl.css	428 B	0 B
build/editor/editor-styles.css	431 B	0 B
build/editor/index.js	43.6 kB	0 B
build/editor/style-rtl.css	3.49 kB	0 B
build/editor/style.css	3.49 kB	0 B
build/element/index.js	4.64 kB	0 B
build/escape-html/index.js	733 B	0 B
build/format-library/index.js	7.29 kB	0 B
build/format-library/style-rtl.css	502 B	0 B
build/format-library/style.css	502 B	0 B
build/hooks/index.js	2.13 kB	0 B
build/html-entities/index.js	622 B	0 B
build/i18n/index.js	3.56 kB	0 B
build/is-shallow-equal/index.js	711 B	0 B
build/keyboard-shortcuts/index.js	2.51 kB	0 B
build/keycodes/index.js	1.91 kB	0 B
build/list-reusable-blocks/index.js	3.12 kB	0 B
build/list-reusable-blocks/style-rtl.css	226 B	0 B
build/list-reusable-blocks/style.css	226 B	0 B
build/media-utils/index.js	5.28 kB	0 B
build/notices/index.js	1.79 kB	0 B
build/nux/index.js	3.4 kB	0 B
build/nux/style-rtl.css	616 B	0 B
build/nux/style.css	613 B	0 B
build/plugins/index.js	2.67 kB	0 B
build/primitives/index.js	1.49 kB	0 B
build/priority-queue/index.js	789 B	0 B
build/redux-routine/index.js	2.84 kB	0 B
build/rich-text/index.js	14.8 kB	0 B
build/server-side-render/index.js	2.67 kB	0 B
build/shortcode/index.js	1.7 kB	0 B
build/token-list/index.js	1.28 kB	0 B
build/url/index.js	4.01 kB	0 B
build/viewport/index.js	1.84 kB	0 B
build/warning/index.js	1.14 kB	0 B
build/wordcount/index.js	1.17 kB	0 B

_{compressed-size-action}

[noisysocks]

I'm OK with this.

[aduth]

Noting that there's some related work happening in #20280 and #20090, at least regarding making the environment consistent between Travis and local development, in order to avoid these weird developer experience barriers of getting the tests to run the same way they do in continuous integration. That said, even as of those changes, the INSTALL_COMPOSER step removed here would still have been left. I think there may still be some alternatives to consider. For me, ideally these occur both transparently and on-demand, either as part of the local environment setup process, or the test run itself. "As part of the test run itself" does happen with these changes, though more wastefully than is often necessary (because it is running composer install every test run).

[noahtallen]

Hm. This doesn't seem to be working for me locally 🤔

~/source/gutenberg(master) » npm run wp-env run composer install -- --no-interaction                                                                                                                                                                         noah.allen@Noahs-MacBook-Pro

> gutenberg@8.1.0-rc.1 wp-env /Users/noah.allen/source/gutenberg
> packages/env/bin/wp-env "run" "composer" "install" "--no-interaction"

⠹ Running `install` in 'composer'.

Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files



⠋ wp-env run <container> [command..]

Runs an arbitrary command in one of the underlying Docker containers, for
example it's useful for running wp cli commands.

Positionals:
  container  The container to run the command on.            [string] [required]
  command    The command to run.                           [array] [default: []]

Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]
  --debug    Enable debug output.                     [boolean] [default: false]

TypeError: Cannot use 'in' operator to search for 'exitCode' in Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files

    at /Users/noah.allen/source/gutenberg/packages/env/lib/cli.js:43:16
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! gutenberg@8.1.0-rc.1 wp-env: `packages/env/bin/wp-env "run" "composer" "install" "--no-interaction"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the gutenberg@8.1.0-rc.1 wp-env script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/noah.allen/.npm/_logs/2020-05-12T21_21_18_745Z-debug.log

[noahtallen]

It still fails just running ./packages/env/bin/wp-env run composer install 🤔 🤔

[aduth]

@noahtallen Yeah, I've seen some similar issues with this lately. Which is odd, because as of these changes, Travis runs the lint through the exact same process.

Do you get the same error when running ./packages/env/bin/wp-env run composer install ?

[noahtallen]

Yep. It looks like docker compose includes a string as the err property of the response, and then wp-env tries to see if there is an error code in that. which doesn't work obviously.

I'll see what happens when we check to make sure that err is an object

[noahtallen]

If we do that:

✖ Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files

Looks a lot nicer, but it doesn't actually seem to be an error!

[aduth]

Do you think the problem is that the presence of any output is being interpreted as an error? Even if it's not an error?

[noahtallen]

It works correctly for other commands, so I'm not sure. I think err and out correspond to stderr/stdout in the container. So maybe composer is doing something unexpected?

[noahtallen]

I think this might just error out when there is nothing to install or update. It probably passes in CI since it does have stuff to install and update

[noahtallen]

this should fix it: #22475

[aduth]

For what it's worth, I'm not entirely loving the changes that were introduced here. At least in common usage, the composer install step adds significant time to what should otherwise be a pretty quick lint command. It's certainly nice to have to avoid confusion for contributors who haven't yet gone through the process of having the composer dependencies prepared yet. But after that initial setup, the direct command is much faster. I've found that I've been tending to run the direct command npm run wp-env run composer run-script lint, which could partly explain why I wasn't encountering the errors.

I'm still not sure what the best solution here would be. It's a bit similar to needing to run npm install occasionally to get or update local dependencies. My experience was that it wasn't as obvious to do this for composer dependencies. Perhaps others encounter the same confusion with npm. It's not like we run npm install as a pre-step to all the Node-based scripts 🤷

[noahtallen]

I agree with you here. It makes no sense why composer takes 2-3 minutes to install when everything is already up to date. Maybe we could expose a package script to run composer install? At that point I think it’s fine to tell people they need to run it once before doing phpcs or phpunit

[aduth]

At that point I think it’s fine to tell people they need to run it once before doing phpcs or phpunit

I do think it's a simple option, but really only if it can be communicated effectively in a way which won't just be utterly confusing.

I haven't looked into it, but one option could be to be more helpful in the error case, if there's some way that the failures associated with not having run this step can include a tip on what the developer needs to do to fix it.

Example:

It appears that the Composer dependencies have not been installed. Run npm run composer-install, then try again.

[noahtallen]

Yeah that would be really useful. Part of me thinks it would be really useful for wp-env too since phpcs and phpunit are very common use cases, but I'm not sure how to integrate those more closely