WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 3 days ago

#47058 reviewing enhancement

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

Reported by: DavidAnderson Owned by: SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-patch needs-testing
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 https://core.trac.wordpress.org/ticket/47046#comment:9 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 (2)

47058.diff (1.5 KB) - added by dkotter 4 weeks ago.
47058.1.diff (1.4 KB) - added by dkotter 3 days ago.

Download all attachments as: .zip

Change History (19)

#1 @Clorith
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months ago

  • Description modified (diff)

#7 @spacedmonkey
15 months ago

  • Component changed from Administration to Site Health

#8 @TimothyBlynJacobs
5 weeks ago

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

#9 @Clorith
5 weeks 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 :)

@dkotter
4 weeks ago

#10 @dkotter
4 weeks 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 weeks 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 weeks ago by knutsp (previous) (diff)

#12 @JeffPaul
8 days ago

  • Keywords has-patch needs-testing added

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


8 days ago

#14 @afragen
8 days 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
8 days 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 8 days ago by SergeyBiryukov (previous) (diff)

@dkotter
3 days ago

#16 @dkotter
3 days 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
3 days ago

  • Owner set to SergeyBiryukov
  • Status changed from reopened to reviewing
Note: See TracTickets for help on using tickets.