Add an option to configure site constants #14371

gziolo · 2019-03-11T12:16:41Z

Description

Replaces #12457 where it was expected to be able to update SCRIPT_DEBUG constant:

When I setup Gutenberg for local development, I'd expect it to load unminified files.

Takes out part of #13452 where it is necessary to be able to set WP_DEBUG constant.

This PR enables an option to make debugging WordPress easier with the existing Docker setup by providing env variables which reflect the same PHP constants used in WordPress:

WP_DEBUG
SCRIPT_DEBUG

See more at https://codex.wordpress.org/Debugging_in_WordPress.

Testing

To enable all available site constant variables run the following:

SCRIPT_DEBUG=true WP_DEBUG=true ./bin/setup-local-env.sh

To disable all available site constant variables run the following:

SCRIPT_DEBUG=false WP_DEBUG=false ./bin/setup-local-env.sh

gziolo · 2019-03-11T12:28:58Z

Yay, it works https://travis-ci.com/WordPress/gutenberg/jobs/183804711#L978-L978.

Thanks @aduth and @pento for inspiration how to tackle it :)

I will polish PR real quick and add missing description and docs.

.travis.yml

gziolo · 2019-03-11T14:21:36Z

bin/install-wordpress.sh

@@ -90,6 +90,24 @@ if [ "$CURRENT_URL" != "http://localhost:$HOST_PORT" ]; then
 	docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm -u 33 $CLI option update siteurl "http://localhost:$HOST_PORT" --quiet
 fi

+# Configure site constants.
+echo -e $(status_message "Configuring site constants...")


There is a lot of code duplication here. Commits which simplify it are warmly welcomed :)

We could use macro-like behaviour with the following:

$ A=hello; B=salut $ VAR=A; eval echo \$$VAR hello $ VAR=B; eval echo \$$VAR salut

ellatrix · 2019-03-12T11:58:55Z

Why don't we just enable them by default?

gziolo · 2019-03-12T12:01:19Z

e2e tests will fail, there are still React warnings coming from class methods which are marked as deprecated in the strict mode :(

ellatrix · 2019-03-12T12:03:43Z

Maybe we can add an alternative command that includes these, in package.json?

ellatrix · 2019-03-12T12:05:35Z

Just saying that when I'm setting up Gutenberg locally, I use it for development purposes. Sounds like we'd benefit from a less verbose setup command.

gziolo · 2019-03-12T12:29:54Z

Feel free to add some changes to this PR if you have some ideas. I just wanted to be able to use SCRIPT_DEBUG myself :) In addition, @aduth wants to enable WP_DEBUG so we could catch PHP errors produced by plugin.

aduth · 2019-03-12T12:32:41Z

e2e tests will fail, there are still React warnings coming from class methods which are marked as deprecated in the strict mode :(

Is it reasonable to just override those temporarily for the end-to-end tests, to disable the script debug?

gziolo · 2019-03-12T13:17:26Z

Is it reasonable to just override those temporarily for the end-to-end tests, to disable the script debug?

I think it makes a lot of sense. It didn't occur to me before :)

CONTRIBUTING.md

gziolo · 2019-03-14T15:42:57Z

I'm seeing the following on Travis:

Error: The constant 'WP_DEBUG_DISPLAY' is not defined in the 'wp-config.php' file.
The command "./bin/setup-local-env.sh" failed and exited with 1 during .

I added a commit which removed the change for WP_DEBUG_DISPLAY: 0eb93b6. It looks like this is a limitation of WP CLI as of today:
https://stackoverflow.com/questions/35711003/wp-cli-toggle-wp-debug/49105953#comment91168718_49105953

gziolo · 2019-03-14T15:51:41Z

Hmmm, it's also being reported for SCRIPT_DEBUG:

Error: The constant 'SCRIPT_DEBUG' is not defined in the 'wp-config.php' file.

I plan to investigate whether we can set those constants inside Docker as explained in docker-library/wordpress#142.

aduth · 2019-03-18T17:37:13Z

bin/bootstrap-env.sh

@@ -1,6 +1,8 @@
 #!/usr/bin/env bash

 WP_VERSION=${WP_VERSION-latest}
+WP_DEBUG=${WP_DEBUG-true}


Possible thought on the error: Is this the correct syntax for assigning the default? Is the colon : not required?

See:

https://stackoverflow.com/a/2013589

https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

Also a bit curious why we don't use a .env file: https://docs.docker.com/compose/env-file/

I followed other occurrences in the file. I think it works correctly, although I personally don't know which syntax would be preferred.

The issue was caused by the fact that PHP didn't have those constants defined and WP CLI's implementation is limited to updating existing constants ... c34d3c0 fixes it.

Ah, re-reading the above document, the colon behavior is explained:

Omitting the colon results in a test only for a parameter that is unset. Put another way, if the colon is included, the operator tests for both parameter’s existence and that its value is not null; if the colon is omitted, the operator tests only for existence.

I expect it should be fine.

The issue was caused by the fact that PHP didn't have those constants defined and WP CLI's implementation is limited to updating existing constants ... c34d3c0 fixes it.

So, to clarify, despite the fact that in c34d3c0 you're hard-coding the constants, bin/install-wordpress.sh will subsequently update them according to the value of the environment variable? And it has to be hard-coded because otherwise WP-CLI will refuse to update them (as they don't otherwise exist in the file)?

Yes, this is complex to explain but correct. Each constant needs to be defined in wp-config.php if we want to be able to update it with WP-CLI.

…ing purpose

gziolo · 2019-03-19T09:05:30Z

It works with the latest changes applied. I think this is an improvement over what we had before so I would be happy if we could land it after some testing.

aduth

This all looks reasonable, but I'm struggling with the previously-noted error in my own environment.

Stopping / removing all existing containers, then running ./bin/setup-local-env.sh, I see:

⇒ docker-compose down
⇒ ./bin/setup-local-env.sh
# ...
Error: The constant 'SCRIPT_DEBUG' is not defined in the 'wp-config.php' file.

Am I missing something, or can you think of some reason why the new environment values specified in docker-compose.yml might not take effect in my environment (would others expect to encounter similar)?

aduth · 2019-03-19T18:46:34Z

bin/bootstrap-env.sh

@@ -1,6 +1,8 @@
 #!/usr/bin/env bash

 WP_VERSION=${WP_VERSION-latest}
+WP_DEBUG=${WP_DEBUG-true}


Ah, re-reading the above document, the colon behavior is explained:

Omitting the colon results in a test only for a parameter that is unset. Put another way, if the colon is included, the operator tests for both parameter’s existence and that its value is not null; if the colon is omitted, the operator tests only for existence.

I expect it should be fine.

aduth · 2019-03-19T18:50:15Z

bin/bootstrap-env.sh

@@ -1,6 +1,8 @@
 #!/usr/bin/env bash

 WP_VERSION=${WP_VERSION-latest}
+WP_DEBUG=${WP_DEBUG-true}


The issue was caused by the fact that PHP didn't have those constants defined and WP CLI's implementation is limited to updating existing constants ... c34d3c0 fixes it.

So, to clarify, despite the fact that in c34d3c0 you're hard-coding the constants, bin/install-wordpress.sh will subsequently update them according to the value of the environment variable? And it has to be hard-coded because otherwise WP-CLI will refuse to update them (as they don't otherwise exist in the file)?

gziolo · 2019-03-20T12:51:41Z

Am I missing something, or can you think of some reason why the new environment values specified in docker-compose.yml might not take effect in my environment (would others expect to encounter similar)?

No idea to be honest. It looks similar to the error we had on Travis before c34d3c0 was added. I personally restarted Docker to test it locally.

aduth

While I'm not able to test it, I strongly suspect my issues are local, and I'd not want to be the reason for this being blocked. Otherwise, everything appears in good order here. I might offer it that another person test it, though am comfortable with it being merged if you're confident about it.

gziolo · 2019-03-21T17:53:58Z

I will restart Docker and try locally again before I merge tomorrow, thanks for your review 💯

gziolo · 2019-03-22T08:04:58Z

I tested locally after system restarted. Everything works as expected.

aduth · 2019-04-05T16:58:29Z

e2e tests will fail, there are still React warnings coming from class methods which are marked as deprecated in the strict mode :(

I created a follow-up task at #14845

gziolo · 2019-04-05T17:46:01Z

Thanks @aduth!

gziolo added [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling labels Mar 11, 2019

gziolo self-assigned this Mar 11, 2019

gziolo requested review from aduth, ajitbohra, nerrad, ntwb and youknowriad as code owners March 11, 2019 12:16

This was referenced Mar 11, 2019

Testing: Fail E2E when page displays warning notices #13452

Merged

Script debug when version controlled #12457

Closed

gziolo requested review from ellatrix and pento March 11, 2019 14:12

gziolo force-pushed the update/docker-install-constants branch from aae4823 to 55bf396 Compare March 11, 2019 14:13

gziolo removed the [Status] In Progress Tracking issues with work in progress label Mar 11, 2019

gziolo added this to the 5.3 (Gutenberg) milestone Mar 11, 2019

gziolo commented Mar 11, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

gziolo commented Mar 11, 2019

View reviewed changes

gziolo requested review from mcsf and oandregal March 12, 2019 11:19

mcsf reviewed Mar 13, 2019

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

gziolo force-pushed the update/docker-install-constants branch from 7a1992c to 45b7fdf Compare March 14, 2019 15:18

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

aduth reviewed Mar 18, 2019

View reviewed changes

gziolo and others added 6 commits March 19, 2019 09:44

Add an option to configure site constants

25d62db

Remove temporary override for one of the Travis jobs added for debugg…

f1678be

…ing purpose

Update CONTRIBUTING.md

4e8f114

Set all flags to true by default, disable on demand

7ecc2ba

Remove WP_DEBUG_DISPLAY override

5e4b14d

Enable WP_DEBUG and SCRIPT_DEBUG

c34d3c0

gziolo force-pushed the update/docker-install-constants branch from cb9b766 to c34d3c0 Compare March 19, 2019 08:45

aduth reviewed Mar 19, 2019

View reviewed changes

aduth approved these changes Mar 21, 2019

View reviewed changes

gziolo merged commit 2ebc98e into master Mar 22, 2019

gziolo deleted the update/docker-install-constants branch March 22, 2019 08:04

gziolo added this to the 5.4 (Gutenberg) milestone Mar 22, 2019

This was referenced Apr 1, 2019

Bump plugin version to 5.4.0-rc.1 #14735

Merged

Bump plugin version to 5.4.0 #14783

Merged

This was referenced Apr 5, 2019

Upgrade to React 16.6 stable APIs #11360

Closed

Testing: Use SCRIPT_DEBUG in end-to-end test environment #14845

Open

aduth mentioned this pull request Jun 5, 2019

adds selection persistence on sidebar tab switch #15583

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to configure site constants #14371

Add an option to configure site constants #14371

gziolo commented Mar 11, 2019 •

edited

Loading

gziolo commented Mar 11, 2019

gziolo Mar 11, 2019

mcsf Mar 13, 2019

ellatrix commented Mar 12, 2019

gziolo commented Mar 12, 2019

ellatrix commented Mar 12, 2019

ellatrix commented Mar 12, 2019

gziolo commented Mar 12, 2019

aduth commented Mar 12, 2019

gziolo commented Mar 12, 2019 •

edited

Loading

gziolo commented Mar 14, 2019

gziolo commented Mar 14, 2019 •

edited

Loading

aduth Mar 18, 2019

aduth Mar 18, 2019

gziolo Mar 19, 2019

aduth Mar 19, 2019

aduth Mar 19, 2019

gziolo Mar 20, 2019

gziolo commented Mar 19, 2019

aduth left a comment

aduth Mar 19, 2019

aduth Mar 19, 2019

gziolo commented Mar 20, 2019

aduth left a comment

gziolo commented Mar 21, 2019

gziolo commented Mar 22, 2019

aduth commented Apr 5, 2019

gziolo commented Apr 5, 2019

Add an option to configure site constants #14371

Add an option to configure site constants #14371

Conversation

gziolo commented Mar 11, 2019 • edited Loading

Description

Testing

gziolo commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Mar 12, 2019

gziolo commented Mar 12, 2019

ellatrix commented Mar 12, 2019

ellatrix commented Mar 12, 2019

gziolo commented Mar 12, 2019

aduth commented Mar 12, 2019

gziolo commented Mar 12, 2019 • edited Loading

gziolo commented Mar 14, 2019

gziolo commented Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Mar 19, 2019

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Mar 20, 2019

aduth left a comment

Choose a reason for hiding this comment

gziolo commented Mar 21, 2019

gziolo commented Mar 22, 2019

aduth commented Apr 5, 2019

gziolo commented Apr 5, 2019

gziolo commented Mar 11, 2019 •

edited

Loading

gziolo commented Mar 12, 2019 •

edited

Loading

gziolo commented Mar 14, 2019 •

edited

Loading