PHPCS: Empty FOREACH statement detected

[dhruvang21]

Empty foreach statement is present in the wp_replace_in_html_tags function on line 757 in the file formatting.php of wp-includes folder

[TobiasBg]

Hi @dhruvang21!
Thanks for the ticket! However, while the foreach body is empty, these lines can not be removed as they do have a purpose:
They make the $needle and $replace variables available for the lines following this loop.

So, instead of removing the lines, maybe a clarifying comment would be more reasonable.

[jrf]

@TobiasBg Better yet, use the correct syntax in that case...
foreach ( $replace_pairs as $needle => $replace );

[TobiasBg]

Thanks, @jrf!

Indeed, it appears that this syntax was originally used, but changed (semi-automatically) in https://core.trac.wordpress.org/changeset/42343/trunk/src/wp-includes/formatting.php some years ago.

[mi5t4n]

Can't we do this, if phpcs is throwing error.

<?php
$needle = current( array_keys( $replace_pairs ) );
$replace = current( array_values( $replace_pairs ) );

[TobiasBg]

From a functionality point of view, this would probably work, but given that the foreach loop is an a code section that aims to optimize the function run, that speed gain would likely be impacted.

Most likely, phpcs won't actually "complain" about jrf's suggestion (and if so, a // phpcs:ignore comment would be appropriate, I think) but only about the current syntax with the empty {} part.

[jrf]

Most likely, phpcs won't actually "complain" about jrf's suggestion (and if so, a phpcs:ignore comment would be appropriate, I think) but only about the current syntax with the empty {} part.

I vaguely remember fixing the sniff which would have (unnecessarily) added the braces all those years ago since, so I don't expect PHPCS to complain, but can check and confirm later.

[SergeyBiryukov]

@jrf @TobiasBg Since this code only runs when $replace_pairs has a single element, and we have a polyfill for array_key_first()​ that also uses a foreach loop, how about this instead?

$needle  = array_key_first( $replace_pairs );
$replace = reset( $replace_pairs );

Might clarify any future confusion.

[jrf]

Replying to jrf:

Most likely, phpcs won't actually "complain" about jrf's suggestion (and if so, a phpcs:ignore comment would be appropriate, I think) but only about the current syntax with the empty {} part.

I vaguely remember fixing the sniff which would have (unnecessarily) added the braces all those years ago since, so I don't expect PHPCS to complain, but can check and confirm later.

Looks like this was ​fixed for cases where it was considered useful, but not for `foreach`, though there is a ​PR lingering which @rodrigosprimo was working on splitting up, which would address this.

However, as @SergeyBiryukov pointed out:

Since this code only runs when $replace_pairs has a single element...

I'd like to suggest a variant of @SergeyBiryukov patch which would avoid the internal array pointer reset:

$needle  = array_key_first( $replace_pairs );
$replace = $replace_pairs[$needle];

Trac ticket:

[TobiasBg]

I like @jrf's suggestion as it's concise and easily readable. The previously/currently used foreach approach might be efficient, but its intention is overlooked easily. To improve readability, changing this makes sense. ~@dhruvang21 or @mi5t4n, would you maybe like to prepare this as an updated patch?~ Nevermind, I missed that @mi5t4n has already updated the PR. Thanks a lot!

[dhruvang21]

@TobiasBg i was just testing the above mentioned patch and it is working fine and saw that the PR is already added.

[mi5t4n]

@TobiasBg Glad to help.

[SergeyBiryukov]

#58357 was marked as a duplicate.

[SergeyBiryukov]

In 58889:

Coding Standards: Replace an empty foreach loop in wp_replace_in_html_tags().

This aims to clarify the intention of the code and improve readability.

Follow-up to [33359].

Props jrf, TobiasBg, mi5t4n, dhruvang21, mayura8991, nadimcse, Presskopp, SergeyBiryukov.
Fixes #61860.

Thanks for the PR! Merged in r58889.