Repository navigation
Eliminate usage of empty() construct - #1803
Conversation
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #1803 +/- ##
==========================================
+ Coverage 65.85% 65.89% +0.03%
==========================================
Files 88 88
Lines 6851 6878 +27
==========================================
+ Hits 4512 4532 +20
- Misses 2339 2346 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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. |
westonruter
left a comment
There was a problem hiding this comment.
Looking mostly good! Just some minor feedback.
| * @since 1.8.0 | ||
| * | ||
| * @param bool $use_output_buffer Whether to use an output buffer. | ||
| * @param bool $enabled Whether to use an output buffer. |
| } | ||
|
|
||
| if ( empty( $option_name ) ) { | ||
| if ( '' === $option_name ) { |
There was a problem hiding this comment.
If someone passed an option_name of '0' then this will pass through the condition since empty( '0' ) === true: https://3v4l.org/LurUe
I don't think this is a problem here though 😄
| } | ||
|
|
||
| if ( empty( $image['file'] ) ) { | ||
| if ( ! isset( $image['file'] ) ) { |
There was a problem hiding this comment.
Technically:
| if ( ! isset( $image['file'] ) ) { | |
| if ( ! isset( $image['file'] ) || '' === $image['file'] ) { |
However, in practice it seems that if $image is not a WP_Error then really the file should always be set. So this entire if statement seems it might be worth removing.
There was a problem hiding this comment.
I think it might be safer to leave the check in because of the image_make_intermediate_size filter applied to the file key. Let me know if you feel strongly about removing it!
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
felixarntz
left a comment
There was a problem hiding this comment.
@ShyamGadde This looks good, thank you for working on this! Just a few small notes on potential improvements.
Co-authored-by: Felix Arntz <flixos90@gmail.com>
Co-authored-by: Felix Arntz <flixos90@gmail.com>
Co-authored-by: Felix Arntz <flixos90@gmail.com>
felixarntz
left a comment
There was a problem hiding this comment.
Thanks for the updates @ShyamGadde, this basically looks good to me.
Two more things that I missed in the review before. Would be great if you could address them, but I will preemptively approve.
Co-authored-by: Felix Arntz <flixos90@gmail.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
westonruter
left a comment
There was a problem hiding this comment.
Just a nit suggestion left, but pre-approving!
Co-authored-by: Weston Ruter <westonruter@google.com>
Summary
Fixes #1219
Relevant technical choices
This PR eliminates the use of
empty()in the following plugins:dominant-color-imagesperformance-labwebp-uploads