Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#47058 closed enhancement (fixed)

Site Health: does not distinguish between production / staging / development sites

Reported by: davidanderson's profile DavidAnderson Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-patch commit
Focuses: Cc:

Description (last modified by SergeyBiryukov)

In a ticket focussed on a different issue - #47046 - the problem was raised that if WP_DEBUG is on, then the "Site Health Check" will raise this as a "critical security" issue. The current justification for that is given in is "You should not be running WP_DEBUG on a production site".

However, currently Site Health Check has no concept of whether a site is production, staging or development. It just shows the message.

Arguably, if WP_DEBUG is on (and in the ticket above, it was on localhost), that's partial evidence that it's not a production site. We don't want users to ultimately learn that "Site Health Check is full of unhelpful approximations, basically, just ignore half of what it says".

Some possible solutions:

  • Remove the check entirely
  • Don't label it as a "Critical Security" issue
  • Introduce a "Production / Development / Staging" switch and then make tests behave different depending upon the value

Attachments (3)

47058.diff (1.5 KB) - added by dkotter 4 years ago.
47058.1.diff (1.4 KB) - added by dkotter 4 years ago.
47058.2.diff (1.8 KB) - added by dkotter 4 years ago.

Download all attachments as: .zip

Change History (26)

#1 @Clorith
5 years ago

  • Keywords site-health close added
  • Summary changed from Site Health Check: does not distinguish between production / staging / development sites to Site Health: does not distinguish between production / staging / development sites

I honestly view this as a non-issue, if you are on a development site, you are unlikely to be looking up the site health of your setup.

#2 @DavidAnderson
5 years ago

you are unlikely to be looking up the site health of your setup

There are loads of self-taught wannabe developers who hear "try out WP, it's pretty easy", and who do so. And that's a good thing. WP core should hand out accurate information, not "accurate information as long as the user is skilled enough to understand when it isn't accurate information". That's irresponsible.

#3 @joyously
5 years ago

I agree this is misleading. I vote for changing the wording, so that it tells the user that DEBUG is on and warns that it is not a good idea for a production site. But it shouldn't be critical.
Sometimes we need to turn it on to see the error and that's likely when we are looking at Site Health also.

#4 @Clorith
5 years ago

If you are using it for debugging on a production site, that's fine, but you should be informed that it's on and you should be aware of the implications of it being on. A large portion of users who turn it on in production do not know of this and just leave it there.

If novice users are using it locally, they should be informed that debug should not be left on unsupervised, regardless of what the state of the site is in that case, to help educate them.

If you want to suggest better wording, go for it, we're always open to improving and iterating on things :)

#5 @Clorith
5 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

There's been no further reasoning for removing this, so I'm closing this for now. Should we find feedback to the contrary after the release, we'll reconsider at that time.

#6 @SergeyBiryukov
5 years ago

  • Description modified (diff)

#7 @spacedmonkey
5 years ago

  • Component changed from Administration to Site Health

#8 @TimothyBlynJacobs
4 years ago

@Clorith should we reconsider this now that we have the notion of environments?

#9 @Clorith
4 years ago

  • Keywords site-health removed
  • Milestone set to 5.6
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Type changed from defect (bug) to enhancement

I think that would have merit now, yes. There's a few checks we can change the criteria for depending on environment I believe (for example updates don't play in on a dev site, but on a stage/prod they should), little things like that.

It's not a pressing issue, so I'll milestone it for 5.6 though :)

4 years ago

#10 @dkotter
4 years ago

I've added a patch that adds a new is_development_environment method in the WP_Site_Health class. This method uses the new wp_get_environment_type function, to check if we're in a development environment. It also checks if the URL us using localhost (which is a check used a couple times already in this class).

If either of those checks match, we return true in that method. I've then implemented this method in the debug mode test, so if a development environment is being used, we change the status from critical to recommended. We do this in both the WP_DEBUG_LOG check and the WP_DEBUG_DISPLAY check.

Note, if we don't want to add a new method, this code could be added inline in both places. I figured to avoid duplicate code, I'd extract that to make it more reusable.

This made the most sense to me, changing that to a recommended notice on development environments instead of critical, but this could be taken further, if we think updating the message would also be helpful.

In addition to changing the status here, we could also change the message, if there's more appropriate language for this check on development environments.

Also, with this new method, it's easy to add this check within other tests here, if needed. I know it was mentioned maybe updates would be one we might want to alter for development environments, though not exactly sure what we would want to change there.

Open to any suggestions or thoughts on how I implemented this though.

#11 @knutsp
4 years ago

A problem with changing how the outcome of tests are treated is that they may come as a surprise when site enters production, and that defeats the purpose of staging (and developing). One can also argue failing tests should be considered more severe in non-production than production.

Some production sites are completely immutable when it comes to the software, so then it's too late and must go back to staging to fix it. So it's very important that no tests are skipped or hidden in other environments.

When I have a staging site mirroring my production site, my production site has no updates, installation of plugins, swithing of themes or theme/plugin editing. Admin can only handle content and users. No Site Health visible or tests running.

A fresh amateur site, as most are, should be considered a development type, as it's open to change by installing plugins to try, change theme and all WP can offer. In development don't exclude production, it's the other way around.

I think core should be very, very careful about letting this new environment type influence anything by default, but let developers define that entirely, for the time being.

It seems to me this feature was added at a nice to have basis, without a clear goal of what it should solve. So we have to find out afterwards. I hope some can make a write-up about how it can be made useful, from the core perspective, beyond standardizing the type names.

Last edited 4 years ago by knutsp (previous) (diff)

#12 @JeffPaul
4 years ago

  • Keywords has-patch needs-testing added

This ticket was mentioned in Slack in #core-site-health by jeffpaul. View the logs.

4 years ago

#14 @afragen
4 years ago

Since the patch tests for localhost should we also test for
in_array( wp_dev_environment_type(), array( 'development', 'local' ), true )
and not just for development?

#15 @SergeyBiryukov
4 years ago

Looking at 47058.diff, I'm not sure we should check for the localhost URL specifically, as a local test site can have pretty much any URL, e.g. http://develop.wordpress.test/.

Just checking for in_array( wp_get_environment_type(), array( 'development', 'local' ), true ) seems enough.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

4 years ago

#16 @dkotter
4 years ago

Updated the environment check in 47058.1.diff

I've removed the localhost check as well, though note the reason I had initially put this in place is we are already doing that localhost check in a couple places within that site health class, specifically for the https test.

#17 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from reopened to reviewing

This ticket was mentioned in Slack in #core-site-health by jeffpaul. View the logs.

4 years ago

#19 @Clorith
4 years ago

  • Keywords needs-refresh added; needs-testing removed

I like the idea of a function to check if it's a dev environment, to determine if certain checks should run, so I'd love to keep that, and the check it performs makes sense to me as well.

I do want to keep the warning from @knutsp in mind as well, that we should be careful about what we do and don't change based on environments, and which environments. With that in mind, I think we should start by replacing the localhost checks with the environment checks, which should be more reliable in that scenario.

I also think it's reasonable to not call WP_DEBUG being enabled as critical on a dev/localhost setup, as it's expected to be on there for most users, I would still like to keep the DEBUG_LOG check as is though, because sensitive information may still end up in a public file when that is enabled (I know I will happily use error_log() to write debug data to the file, over displaying it publicly when doing some deep-dive debugging, because I know I'm in control of the moment, but everyone might not be).

4 years ago

#20 @dkotter
4 years ago

In 47058.2.diff, I no longer change the status of the WP_DEBUG_LOG warning. I've also removed the two localhost checks that already existed and replaced those with the new development environment check added as part of this ticket.

This ticket was mentioned in Slack in #core-site-health by jeffpaul. View the logs.

4 years ago

#22 @Clorith
4 years ago

  • Keywords commit added; needs-refresh removed

47058.2.diff is looking good, thanks for the updated patch! I think can get this in :)

#23 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 49237:

Site Health: Introduce the WP_Site_Health::is_development_environment() method.

This allows Site Health tests to check if the current environment type is set to development or local.

Use the new method:

  • In HTTPS tests, instead of a hardcoded check for localhost.
  • In WP_DEBUG and WP_DEBUG_DISPLAY tests, to set the status to recommended instead of critical.

Props dkotter, Clorith, DavidAnderson, joyously, knutsp, afragen, SergeyBiryukov.
Fixes #47058.

Note: See TracTickets for help on using tickets.