Eliminate usage of `empty()` construct by ShyamGadde · Pull Request #1803 · WordPress/performance

ShyamGadde · 2025-01-16T12:11:40Z

Summary

Fixes #1219

Relevant technical choices

This PR eliminates the use of empty() in the following plugins:

dominant-color-images
performance-lab
webp-uploads

Signed-off-by: Shyamsundar Gadde <[email protected]>

codecov · 2025-01-16T12:19:53Z

Codecov Report

Attention: Patch coverage is 78.37838% with 16 lines in your changes missing coverage. Please review.

Project coverage is 65.89%. Comparing base (3ce0f45) to head (2a93cf4).
Report is 19 commits behind head on trunk.

Files with missing lines	Patch %	Lines
plugins/webp-uploads/hooks.php	73.68%	10 Missing ⚠️
plugins/webp-uploads/deprecated.php	0.00%	5 Missing ⚠️
plugins/dominant-color-images/hooks.php	80.00%	1 Missing ⚠️

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

Flag	Coverage Δ
multisite	`65.89% <78.37%> (+0.03%)`	⬆️
single	`38.61% <77.02%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-01-24T19:23:53Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ShyamGadde <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: swissspidy <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter

Looking mostly good! Just some minor feedback.

westonruter · 2025-01-25T01:39:12Z

plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php

 		 * @since 1.8.0
 		 *
-		 * @param bool $use_output_buffer Whether to use an output buffer.
+		 * @param bool $enabled Whether to use an output buffer.


Good catch.

westonruter · 2025-01-25T01:46:37Z

plugins/performance-lab/includes/site-health/audit-autoloaded-options/hooks.php

 	}

-	if ( empty( $option_name ) ) {
+	if ( '' === $option_name ) {


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 😄

plugins/performance-lab/includes/site-health/audit-enqueued-assets/hooks.php

westonruter · 2025-01-25T01:54:01Z

plugins/webp-uploads/helper.php

 	}

-	if ( empty( $image['file'] ) ) {
+	if ( ! isset( $image['file'] ) ) {


Technically:

Suggested change

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.

Updated the check in 375a1ed.

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!