Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect development mode constant from being re-defined #14165

Merged
merged 1 commit into from
Apr 24, 2019
Merged

Protect development mode constant from being re-defined #14165

merged 1 commit into from
Apr 24, 2019

Conversation

simison
Copy link
Member

@simison simison commented Feb 28, 2019

When defining GUTENBERG_DEVELOPMENT_MODE in wp-config.php and running Gutenberg master directly from the repository, we'd see PHP warnings:

[28-Feb-2019 11:39:14 UTC] PHP Notice:  Constant GUTENBERG_DEVELOPMENT_MODE already defined in /var/www/html/wp-content/plugins/gutenberg/gutenberg.php on line 13
[28-Feb-2019 11:39:14 UTC] PHP Stack trace:
[28-Feb-2019 11:39:14 UTC] PHP   1. {main}() /var/www/html/wp-cron.php:0
[28-Feb-2019 11:39:14 UTC] PHP   2. require_once() /var/www/html/wp-cron.php:39
[28-Feb-2019 11:39:14 UTC] PHP   3. require_once() /var/www/html/wp-load.php:37
[28-Feb-2019 11:39:14 UTC] PHP   4. require_once() /var/www/html/wp-config.php:89
[28-Feb-2019 11:39:14 UTC] PHP   5. include_once() /var/www/html/wp-settings.php:342
[28-Feb-2019 11:39:14 UTC] PHP   6. define() /var/www/html/wp-content/plugins/gutenberg/gutenberg.php:13

We basically wanted to install Gutenberg (plugin version, not this repo) in development mode to Jetpack-Docker development environment by default. That's not a problem until someone runs Gutenberg from master and then gets these PHP warnings.

Came up in Automattic/jetpack#11439 (review)

Now, I get this is in between AUTO-GENERATED DEFINES comments so there's probably a place somewhere else where I should PR. Happy to do so. :-)

How has this been tested?

Define constant in your wp-config.php or wp-content/mu-plugins folder and observe Gutenberg not trying to double-define it.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label Mar 11, 2019
@gziolo gziolo added this to the 5.3 (Gutenberg) milestone Mar 11, 2019
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Mar 11, 2019
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
When defining `GUTENBERG_DEVELOPMENT_MODE` in `wp-config.php` and running Gutenberg `master` directly from the repository, we'd see PHP warnings:

```
[28-Feb-2019 11:39:14 UTC] PHP Notice:  Constant GUTENBERG_DEVELOPMENT_MODE already defined in /var/www/html/wp-content/plugins/gutenberg/gutenberg.php on line 13
[28-Feb-2019 11:39:14 UTC] PHP Stack trace:
[28-Feb-2019 11:39:14 UTC] PHP   1. {main}() /var/www/html/wp-cron.php:0
[28-Feb-2019 11:39:14 UTC] PHP   2. require_once() /var/www/html/wp-cron.php:39
[28-Feb-2019 11:39:14 UTC] PHP   3. require_once() /var/www/html/wp-load.php:37
[28-Feb-2019 11:39:14 UTC] PHP   4. require_once() /var/www/html/wp-config.php:89
[28-Feb-2019 11:39:14 UTC] PHP   5. include_once() /var/www/html/wp-settings.php:342
[28-Feb-2019 11:39:14 UTC] PHP   6. define() /var/www/html/wp-content/plugins/gutenberg/gutenberg.php:13
```
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable enhancement. I had to refresh my history on how these auto-generated lines are treated, since at a glance it seemed the wrong place to make the change. On further investigation, however, it appears correct. It's located here so that it becomes removed as part of the plugin build and replaced instead with production defines (source). I'm not positive about the conventionality of defined( 'x') or define( 'x', true );, but it's valid in assigning-when-not-assigned, and seems preferable to keep aligned to a one-per-line define of this segment of code.

I rebased the branch since it was quite stale and Travis would not complete or restart.

@aduth aduth merged commit 65ef499 into WordPress:master Apr 24, 2019
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants