Opened 6 years ago
Closed 4 years ago
#47058 closed enhancement (fixed)
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 commit |
Focuses: | Cc: |
Description (last modified by )
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 (3)
Change History (26)
#1
@
6 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
#2
@
6 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
@
6 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
@
6 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
@
6 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.
#9
@
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 :)
#10
@
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
@
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.
This ticket was mentioned in Slack in #core-site-health by jeffpaul. View the logs.
4 years ago
#14
@
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
@
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.
#16
@
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.
This ticket was mentioned in Slack in #core-site-health by jeffpaul. View the logs.
4 years ago
#19
@
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).
#20
@
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
@
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 :)
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.