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

Add WP_DEVELOPMENT_MODE constant plus related filter and function #4565

Closed

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Jun 7, 2023

This PR implements a proposed path forward for the below ticket and also applies it in a first usage, replacing the somewhat misused WP_DEBUG lookups to modify theme.json caching behavior.

Tests still need to be added.

Trac ticket: https://core.trac.wordpress.org/ticket/57487


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

src/wp-includes/load.php Outdated Show resolved Hide resolved
@azaozz
Copy link
Contributor

azaozz commented Jun 9, 2023

@felixarntz Thanks for the PR!

Wondering if WP_DEBUG, SCRIPT_DEBUG, etc. constants should also be set when setting WP_DEVELOPMENT_MODE? Seems they should be? Also, perhaps if WP_DEVELOPMENT_MODE is set and WP_DEBUG_DISPLAY is false, can set WP_DEBUG_LOG to true?

Any other constants and "shortcuts" that may make sense to be set for dev. mode?

@spacedmonkey spacedmonkey self-requested a review June 12, 2023 11:47
@felixarntz
Copy link
Member Author

@azaozz

Wondering if WP_DEBUG, SCRIPT_DEBUG, etc. constants should also be set when setting WP_DEVELOPMENT_MODE? Seems they should be? Also, perhaps if WP_DEVELOPMENT_MODE is set and WP_DEBUG_DISPLAY is false, can set WP_DEBUG_LOG to true?

I think for WP_DEBUG that would be reasonable, since it is already conditionally set based on the environment type, and it's clear that any development mode should also result in WP_DEBUG enabled by default, as it is recommended to develop with debugging on. I've updated that in 093d74b.

For the other suggestions, I would suggest to handle that separately since those would be changes that require more discussion. Any of those changes we could have also made e.g. during introduction of WP_ENVIRONMENT_TYPE, but we didn't, so I think that should be decoupled.

Copy link
Contributor

@azaozz azaozz left a comment

Choose a reason for hiding this comment

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

LGTM! Using a PHP global to make testing possible seems better than using a filter.

@felixarntz
Copy link
Member Author

Committed via https://core.trac.wordpress.org/changeset/56042 🎉

@felixarntz felixarntz closed this Jun 26, 2023
@oandregal
Copy link
Member

@felixarntz @azaozz I've noticed the metric "server time (total) for block themes" has improved dramatically after this change:

Captura de ecrã de 2023-06-27 16-32-24

Is this expected? Could you elaborate why this happens? I'd be super excited if this is a real improvement :)

@felixarntz
Copy link
Member Author

@oandregal Interesting. This change by itself is 100% not impacting performance. So question is why did the number change so much?

My first hunch is that we may be running the performance tests with WP_DEBUG enabled?? If yes, that would explain it, since before this change WP_DEBUG prevented caching on several block theme relevant functions, which was always something we wanted to change, and did here. So basically now the caches are enabled also if WP_DEBUG is enabled, so that may make things seem faster in the metrics, but really there's no difference. Those are effectively the WP 6.2 performance wins only showing up now 😁

Would be good to validate that the above is in fact the case. Are we running performance tests with WP_DEBUG?

@oandregal
Copy link
Member

Looking at the CI job, I can learn that ImageMagic supports 247 formats, though I am unable to find the values of WP_DEBUG or SCRIPT_DEBUG.

The performance workflow doesn't seem to set anything different from the defaults, hence, it sounds WP_DEBUG and SCRIPTS_DEBUG would be true.

I'd appreciate if someone with more experience in GitHub CI/wp-env than I have could double-check this.

@oandregal
Copy link
Member

Actually, scratch what I said above: I thought WordPress core used wp-env but it uses a totally different mechanism. It seems to use this .env file that sets WP_DEBUG and SCRIPTS_DEBUG to true.

@azaozz
Copy link
Contributor

azaozz commented Jun 28, 2023

but it uses a totally different mechanism. It seems to use this .env file that sets WP_DEBUG and SCRIPTS_DEBUG to true.

The values from that .env file are only used for the initial install (afaik). The actual wp-config.php file is in your WP installation dir, can tweak it as you want :)

@felixarntz
Copy link
Member Author

@oandregal Interesting, I just spotted that the numbers on https://www.codevitals.run/project/wordpress all went back up again a few commits later. Any idea why? Maybe that wasn't related to this commit at all? 🤔

@oandregal
Copy link
Member

@felixarntz note there were two drops and one uptick. The metrics haven't recovered from the first drop and it points to this PR:

image

I think it's related. Both core and Gutenberg had WP_DEBUG to true in the performance tests as far as I can see. I'm fixing Gutenberg in WordPress/gutenberg#52016 but I don't know how to fix core's.

@felixarntz
Copy link
Member Author

@oandregal Ah my bad, thanks for clarifying, I somehow missed that there were 2 drops like that.

@oandregal
Copy link
Member

@felixarntz @azaozz any progress on why this PR affected TTFB? Also, making sure we use production environment values would be important. I'm happy to create a trac issue for this if that's preferred.

@felixarntz
Copy link
Member Author

@oandregal I thought it was #4565 (comment)? If the tests are run with WP_DEBUG=true, it means that the results of several functions weren't cached prior to this change. But now we're no longer relying on WP_DEBUG, but the new WP_DEVELOPMENT_MODE, which was introduced partially to address exactly those TODO comments. Since the performance tests (I assume) don't set WP_DEVELOPMENT_MODE, now the results are cached, and therefore faster.

@joemcgill
Copy link
Member

@felixarntz and @oandregal, I'm just catching up on this conversation and confirmed that the Performance GH workflow in the core repo uses the docker environment bundled in Core, rather than the wp-env package, which means it also is configured by the included .env file in the core repo, which does set both WP_DEBUG and SCRIPT_DEBUG to true, and sets WP_DEVELOPMENT_MODE to core. These are definitely oversights, in my opinion that should be easy to fix. I can make a new ticket to address these issues.

I think a bit of investigation into why each of those large shifts in performance numbers were recorded, would be useful. It seems like the initial drop was indeed caused by the addition of WP_DEVELOPMENT_MODE, but am unsure why the second drop happened on this commit. The final large increase happened during this package update, which could be several factors, like merged PHP changes related to block library code, but it's hard to pinpoint.

@joemcgill
Copy link
Member

Ticket created: https://core.trac.wordpress.org/ticket/58825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants