-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Improve error handling block pattern #5462
Improve error handling block pattern #5462
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey I am not sure I understand the problem this is trying to fix. Can you outline the circumstances that lead to the bug please? I read through the ticket but do not understand the problem: If WP_DEVELOPMENT_MODE
is set correctly to theme
, there shouldn't be a problem with this? Or, how can this happen even with the constant correctly set?
The bug comes from when a block pattern file is removed or renamed, when developer mode is not active. A valid use case for those that using FTP files to deploy changes. If a user / developers removes or renames, this generates a PHP warning, as the file does not exist. This is because the new pattern cache, uses the keys in the array to do include. What this patch does, is if the file does not exist, now we silence the error, it will see if the file is empty and then flush the pattern cache. This means the in the extremely rare cache of the file magically not existing, it will force a cache refresh. If the file has been removed, then the cache will regenerate and remove the file from. If it is a case that file was unavaliable while a file is being uploaded, the regeneration should be in time for file to be able. This PR is being overly careful while files are being updated or if files are renamed. |
@spacedmonkey Thanks for clarifying. I agree with the idea behind it, however I have one concern with your reply:
Deploying a new version of a theme via FTP or git is totally reasonable and should work given the caches only are used if the theme version matches. However, deploying changes to specific files in a theme is literally why Regarding the concrete fix, I think there is a better solution, or at least an additional fix to add: We should solve this also at the caching level itself: In $pattern_data = $theme->get_pattern_cache();
if ( is_array( $pattern_data ) {
if ( $can_use_cached ) {
return $pattern_data;
}
$theme->delete_pattern_cache();
} This ensures that, as soon as someone enables theme development mode, existing caches are wiped. |
src/wp-includes/block-patterns.php
Outdated
// The actual pattern content is the output of the file. | ||
ob_start(); | ||
include $dirpath . $file; | ||
@include $file_path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave the decision on the error control operator to others, though it is worth reminding it is highly discouraged by the WP coding standards.
I'm not sure why but I see the PHP Coding Standards checks on the related GitHub workflow run with the flag -n
, which doesn't print warnings. It seems it was the same case also on Travis CI. I'm not sure I understnd why we have coding standards error and warnings when then the warnings are not reported.
@jrfnl and @desrosj may know more.
Instead, commands like grunt lint:php
run without the -n
flag and this warning will be a new warning.
I do see in core 22 occurrences of:
phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged
but I'm unsure what should be done in this case. However, at the very least there shoud be a comment to explain why the @
is being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only way to handle php warning here. Adding this error is silenced, it will result in an empty string being return and caches invalidated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why but I see the PHP Coding Standards checks on the related GitHub workflow run with the flag -n, which doesn't print warnings. It seems it was the same case also on Travis CI. I'm not sure I understand why we have coding standards error and warnings when then the warnings are not reported.
When automated checking for coding standards issues in PHP files was introduced in WP, everything that could be was bulk fixed. However, there were a significant number of issues (IIRC some 5000 of them) which needed manual review/attention.
To still allow the automated checking to be enabled, if was decided to ignore those for CI by changing them to warnings ( if these weren't warnings already) and ignoring warnings.
Over time, work has been done on these issues, mostly by @SergeyBiryukov, and the number of issues is much much lower now (~600). The intention is to turn "failing on warnings" on in CI, once the warnings have been brought down to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only way to handle php warning here. Adding this error is silenced, it will result in an empty string being return and caches invalidated.
I don't understand what you are saying here. The @
should not be used if proper preliminary checks can prevent the warning/error. It should only be used if no amount of checking can prevent a PHP warning/error.
Why would a check to see if the $file_path
is a file and is readable before including the file not be possible here ?
If such a check was put in place, the error silencing should be able to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrfnl I believe what @spacedmonkey was referring to is that in the past there have been several instances where file_exists()
checks were leading to performance problems. So it's not that it wouldn't be possible, but it's a performance concern. Technically speaking, why check that the file exists and then include the contents when we can also directly include the contents and by that alone recognize whether the file exists? However, I do understand suppressing PHP errors isn't a good approach either.
I have taken a closer look at this locally, and I think adding a file_exists()
check here is reasonable:
- Past cases of
file_exists()
performance bottlenecks were about checks where the file didn't exist (due to some PHP internals?). - In the function here, it is expected that those file should exist, the scenario we are addressing here is an edge case. So 99+% of
file_exists()
calls here would be on a file that exists. - I benchmarked server response time locally with and without a
file_exists()
check and there was no performance difference at all.
So I agree with the approach of adding file_exists()
as it is a more appropriate solution than suppressing PHP errors, and it appears to not have a relevant performance cost here.
As another reference point of interest: Based on https://3v4l.org/iHkVV, it's actually faster to have a file_exists()
here, even when the file doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in c244c4e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixarntz I understand the concern, but performance should never be a reason to introduce sloppy code. I'm happy to see the file_exists()
added now.
On the note of performance: any time PHP throws an (avoidable) warning/error is actually a performance issue in itself, especially when error logging is on, as the handling of the warning/error via potentially set custom error handlers + the file write for the logging will impact performance.
So even with performance in mind, the general rule of thumb "warnings/errors should be prevented whenever possible instead of silencing them" is still the correct way forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the concern, but performance should never be a reason to introduce sloppy code.
Thanks for having a look and for your feedback @jrfnl I'd totally second that.
Committed in https://core.trac.wordpress.org/changeset/56931 |
Trac ticket: https://core.trac.wordpress.org/ticket/59591
Improve error handling of block pattern registration. Error handling is improved in the following ways.
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.