Do not flag localhost usage in comments

[nielslange]

Description of the Change

In #267, @leoloso reported that the error "Do not use Localhost in your code" also gets triggered when the string "localhost" is used within a comment. This PR updates the corresponding regex to ignore the string "localhost" within comments.

Closes #267

How to test the Change

1. Download and activate this helper-plugin.zip
2. Go to /wp-admin/tools.php?page=plugin-check and check the helper plugin before applying this fix.
3. Verify that the error "Do not use Localhost/127.0.0.1 in your code." appears.
4. Apply this PR and check the helper plugin again.
5. Verify that the error "Do not use Localhost/127.0.0.1 in your code." no longer appears.

Changelog Entry

Fixed - “localhost” in comments no longer raises error.

Credits

Props @nielslange

Checklist:

• I agree to follow this project's Code of Conduct.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests pass.

[bordoni]

Thank you @nielslange, the code you are creating your Pull Request against is the new Codebase, and it wont be released on the plugin for a little bit so we cannot close the #267 using this PR unless the code you base your PR is from the legacy-plugin branch of this repository.

Sorry for the confusion there.

Now talking about the code, I wonder how much of a performance difference this particular approach has compared to the existing one? We need to consider that, since checks will run on plugins that are 10MB big.

[swissspidy]

I think this would be the first sniff using token_get_all(). While it's probably fine, I wonder whether this could be covered by a dedicated PHPCS sniff that can detect disallowed strings more precisely. Maybe such a sniff already exists?

[ernilambar]

Such dedicated PHPCS sniff does not exist. I suggested for such sniff here PHPCSStandards/PHPCSExtra#302 but it has been closed as this would too generalized and very difficult to maintain and support. So I believe PHPCS approach is no go this scenario.

[mukeshpanchal27]

Thank you, @nielslange, for the pull request. I apologize for the delayed review. I have provided some feedback for your consideration.

In response to @swissspidy's feedback, I invested some time in attempting to identify the issue. However, rather than utilizing a "sniff", we can readily achieve the desired result through the code.

[mukeshpanchal27]

Suggested change

Remove extra line.

[mukeshpanchal27]

IMO we should update the code like below:

• Find all the files which have localhost/127.0.0.1 in code base.
• Run token_get_all one filter files.
• Check localhost/127.0.0.1 in tokens.

Code:

protected function check_files( Check_Result $result, array $files ) {
	$php_files = self::filter_files_by_extension( $files, 'php' );
	$file      = self::file_preg_match( '#https?://(localhost|127.0.0.1)#', $php_files );
	if ( $file ) {
		$code   = file_get_contents( $file );
		$tokens = token_get_all( $code );

		foreach ( $tokens as $token ) {
			if ( is_array( $token ) ) {
				if ( in_array( $token[0], array( T_COMMENT, T_DOC_COMMENT ), true ) ) {
					continue;
				}

				if ( preg_match( '#https?://(localhost|127.0.0.1)#', $token[1] ) ) {
					$result->add_message(
						true,
						__( 'Do not use Localhost/127.0.0.1 in your code.', 'plugin-check' ),
						array(
							'code' => 'localhost_code_detected',
							'file' => $file,
						)
					);
					break;
				}
			}
		}
	}
}

[ernilambar]

I used time command to find the time taken for completion of the command. But it seems time gives different output for same command run consecutively. So, I ran commands 3 times and calculate the average time. This does not seem to be reliable test but I did not find much significant drops in performance when trying with three big WordPress plugins.

Before PR:

• WooCommerce - 7.54s
• Yoast SEO - 2.67s
• EDD - 4.99s

After PR:

• WooCommerce - 6.45s
• Yoast SEO - 3.12s
• EDD - 3.76s

[ernilambar]

While checking this PR, I found another issue. #362

[swissspidy]

I previously suggested using a dedicated PHPCS sniff for this, and I think https://github.com/WPTT/WPThemeReview/blob/15684d0852fe90d807c2ae7746dea1302b74b4bd/WPThemeReview/Sniffs/Privacy/ShortenedURLsSniff.php could be a great inspiration here.

[github-actions]

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: nielslange <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: ernilambar <[email protected]>
Co-authored-by: bordoni <[email protected]>
Co-authored-by: davidperezgar <[email protected]>
Co-authored-by: leoloso <[email protected]>

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

[ernilambar]

Since we are now adding custom sniff in PCP itself, I would also recommend to create custom sniff for this issue. Check will be more robust with sniff I believe.

[davidperezgar]

Yes, I'm agree with @ernilambar

[ernilambar]

@nielslange Thanks for the PR and contribution. Closing this now as sniff approach looked more appropriate now.

Closing in favor of #743