WPCS Improvement #1123
WPCS Improvement #1123
Conversation
Chintesh
requested review from
westonruter,
felixarntz and
joemcgill
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| // Retrieve the sanitized value of the 'REQUEST_METHOD' server variable. | ||
| $method = filter_input( INPUT_SERVER, 'REQUEST_METHOD', FILTER_SANITIZE_STRING ); |
There was a problem hiding this comment.
I don't think we should use filter_input() for two reasons:
- It's not used in core.
- It's not able to be unit tested (e.g. by setting
$_SERVER['REQUEST_METHOD']).
You can see there is a unit test failing for the second reason: https://github.com/WordPress/performance/actions/runs/8651242612/job/23721516648?pr=1123#step:9:84
You can see the test setup is overriding $_SERVER['POST_METHOD']:
performance/tests/plugins/optimization-detective/optimization-tests.php
Lines 124 to 130 in 2fd48b0
There was a problem hiding this comment.
Got it, I have Updated this
Thanks @westonruter
| @@ -30,7 +30,7 @@ function auto_sizes_update_image_attributes( $attr ) { | |||
| } | |||
|
|
|||
| // Don't add 'auto' to the sizes attribute if it already exists. | |||
| if ( false !== strpos( $attr['sizes'], 'auto,' ) ) { | |||
| if ( str_contains( $attr['sizes'], 'auto,' ) ) { | |||
There was a problem hiding this comment.
str_contains() is only available as of PHP8. However, since WP 5.9 and later implement a polyfill for this, then this is fine.
| $able = ! ( | ||
| // Since there is no predictability in whether posts in the loop will have featured images assigned or not. If a | ||
| // theme template for search results doesn't even show featured images, then this wouldn't be an issue. | ||
| is_search() || | ||
| // Since injection of inline-editing controls interfere with breadcrumbs, while also just not necessary in this context. | ||
| is_customize_preview() || | ||
| // Since the images detected in the response body of a POST request cannot, by definition, be cached. | ||
| 'GET' !== $_SERVER['REQUEST_METHOD'] || | ||
| ( isset( $method ) && 'GET' !== $method ) || |
There was a problem hiding this comment.
There isn't a need for isset() if the $method variable was set above, is there? Here it's just checking if it is null. There's no string offset going on.
There was a problem hiding this comment.
I have Updated this
Thanks @westonruter
plugins/speculation-rules/helper.php
Outdated
| @@ -93,7 +93,7 @@ function plsr_get_speculation_rules() { | |||
| ), | |||
| ); | |||
|
|
|||
| // Allow adding a class on any links to prevent prerendering. | |||
| // Allow adding a class on any links to prevent pre-rendering. | |||
There was a problem hiding this comment.
I don't think this change should be made since "prerendering" is the term used. See https://developer.mozilla.org/en-US/docs/Glossary/Prerender
There was a problem hiding this comment.
I have Reverted it
Thanks @westonruter
westonruter
added
the
[Type] Enhancement
westonruter
added
no milestone
Summary
Fixes #
Relevant technical choices
I've refactored the code to address PHPCS (PHP CodeSniffer) warnings and improve code readability and standards compliance.
isset()to check if a variable is set before accessing it to avoid illegal string offset warnings.strpos()withstr_contains()for more readable string containment checks.Utilizedfilter_input()with appropriate sanitization for better handling of input data from superglobals.Ensured that all global variables like$_SERVERare properly validated before use to prevent potential security vulnerabilities.These changes aim to enhance the overall quality and maintainability of the codebase by adhering to best practices recommended by PHPCS.