Disable Xdebug by default unless specified by user

[brylie]

Description

Currently, wp-env installs Xdebug regardless of whether the user has requested it. The only installation check is based on PHP version being 7.2+.

Xdebug shouldn't be installed unless specified by the user. Likewise, the Docker file should be more careful when running the pecl install xdebug command, as noted by @noahtallen.

The added complexity of the Xdebug installation is causing some other issues:

• wp-env: start --update fails on pecl install xdebug #30213
• wp-env: docker-compose fails on PHP < 7.2 due to Xdebug #27783
• [wordpress/env] Service 'wordpress' failed to build: The command '/bin/sh -c pecl install xdebug && docker-php-ext-enable xdebug' returned a non-zero code: 1 #34320

How has this been tested?

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @brylie! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[derickr]

It's spelled "Xdebug" (no capital "D").

XDebug shouldn't be installed unless specified by the user.

FWIW, I don't think this is wise, as you're going to make it harder for people to get a working debugging setup.

[brylie]

It's spelled "Xdebug" (no capital "D").

Thanks for pointing that out. I corrected the title and description 😃

FWIW, I don't think this is wise, as you're going to make it harder for people to get a working debugging setup.

From what I understand config.xdebug defaults to off.

This pull request avoids automatically installing Xdebug when config.xdebug === 'off'. When the developer specifies an xdebug value other than off, Xdebug will be installed.

It may be worth pointing out that wp-env/Docker will re-build the development container when enabling Xdebug, so it may be OK to omit the Xdebug installation step in cases where it isn't needed.

[brylie]

There is currently no way to disable the Xdebug installation, even when it is desirable to do so. I am encountering a bug during the Xdebug installation (#34320) step and would just like to bypass the Xdebug installation to continue with my development task.

For simplicity's sake, it is important only to install dependencies that are needed.

[koengabriels]

hope this gets through because I am working for a client that uses wp-env to set up unit tests on their precommit hook
since I am getting this error and have been unable to bypass it... I can not commit anything :(

[brylie]

@koengabriels would you mind taking a close look at the code and possibly testing it on your end? Your peer review would be much appreciated 😃

[koengabriels]

@brylie I have to look into how to do that, I have zero experience with testing a PR from Github or wp-env code base
if I can I will post feedback here, appreciate the time you took to create a pull request for this issue 🙂

[noahtallen]

Left a couple comments, but I think we can move forward with this.

This doesn't change the public API for using xdebug with wp-env, so it isn't any harder to enable with wp-env. 👍

I tested locally, and it will create a wordpress image without xdebug by default, and one with xdebug as soon as the --xdebug flag is passed.

Another note is to run prettier locally to get the lint check passing 👍

[brylie]

@noahtallen, I've incorporated your suggested changes to the nested logic and conditional as well as the suggestion from PR #35667

Are there any other changes you would recommend?

[noahtallen]

This tests well for me, thank you for implementing the suggestions!

I don't have access to merge unless the CI checks are passing. I'd recommend running prettier/eslint again and rebasing the PR on the latest trunk changes.

It would also be great if you could add a line to the changelog (https://github.com/WordPress/gutenberg/blob/trunk/packages/env/CHANGELOG.md) under the "unreleased" section (probably under a new heading ### Enhancement)

Other than that, I think this is good to go 👍

[brylie]

Sorry for the review request bonanza. I tried to rebase changes from trunk into this branch so the CI tasks would succeed.

Would someone with admin rights please help bypass the CI review since most tasks are unrelated to this pull request?

@noahtallen, would you please review the recent changes to this PR? I've refactored the PHP compatibility check into its own function so the dockerFileContents function is single-purpose.

[swissspidy]

Why bypass CI review?

There are some actual lint errors in packages/env/lib/init-config.js that need to get addressed.

[brylie]

Another note is to run prettier locally to get the lint check passing

When I run npm run format-js locally, I get the following error:

$ npm run format-js

> [email protected] format-js
> wp-scripts format-js

sh: 1: wp-scripts: not found

When I have tried running npm install, I get many errors related to Tracker "idealTree:inflate:s" already exists

2021-10-29T08_01_35_754Z-debug.log.txt

I believe these issues are related to the monorepo nature of this project whereby it is necessary to install the entire Gutenberg development stack as well as any other libraries published by @wordpress in order to work on wp-env

#34325

[swissspidy]

Yes it's a monorepo and you'd need to run npm install from the repo root.

You should also be using the latest Node LTS, Node 16 (run nvm install to fix). According to your logs you are still on Node 12

[brylie]

I ran nvm install 16 && nvm use 16:

Running npm install in the monorepo root seems to have produced pretty much the same errors:

2021-10-29T08_48_38_880Z-debug.log.txt

[brylie]

@noahtallen, it seems like the CI checks are all passing or skipped. Would you mind giving this another review?

[noahtallen]

I ran nvm install 16 && nvm use 16:

For future reference, I think this is because gutenberg still requires npm v6. I think there is a project to update that, though.

But now that CI is passing, it shouldn't be a problem for this PR :)

[noahtallen]

The latest changes also test well, thank you!

This change will get released to npm whenever the gutenberg project does its next npm release, though I'm not sure when that will be unfortunately. I think the schedule changes around WordPress release windows