Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AbstractFunctionRestrictions: fix constants being recognized as functions #1863

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Feb 7, 2020

This is a follow-up on #1658 which special cased the curl_version() function for the WP.AlternativeFunctions sniff.

The bug reported to me was that the sniff was throwing an error for the below code:

		$curl = curl_version();

		$ssl_support = true;
		if ( ! $curl['features'] && CURL_VERSION_SSL ) {
			$ssl_support = false;
		}

Turned out, it wasn't throwing an error for the function call to curl_version(). Instead it was (incorrectly) throwing an error for the use of the CURL_VERSION_SSL constant, thinking that was a function call.

I've fixed this now and added a number of additional unit tests.

With an eye on modern PHP, the abstract sniff(s) needs more work, but as PHPCSUtils is expected to have a similar abstract sniff soon which will take all that into account and which we will be able to switch to, I think this will have to do for now.

Commit Summary

AlternativeFunctions: use whitelist instead of special case

The AbstractFunctionRestrictions sniff allows for a whitelist of functions which shouldn't be matched when a wildcard is used.
We may as well use it as it will bow out earlier than special casing the function in the switch.

AbstractFunctionRestrictions: improve matching of function calls

The code as it was could inadvertently match a CONSTANT with the same name as a function.
This has been fixed by adding a check for an open parenthesis after the function name.

Adding that check, however, would break the check on use function import statements. So some additional code has been added to make sure those will still be matched too.

Includes tightening up the regex pattern.

Includes unit tests via the WP.AlternativeFunctions sniff.

jrfnl added 2 commits February 7, 2020 15:31
The `AbstractFunctionRestrictions` allow for a `whitelist` of functions which shouldn't be matched when a wildcard is used.
We may as well use it as it will bow out earlier than special casing the function in the `switch`.
The code as it was could inadvertently match a CONSTANT with the same name as a function.
This has been fixed by adding a check for an open parenthesis after the function name.

Adding that check, however, would break the check on `use function` import statements. So some additional code has been added to make sure those will still be matched too.

Includes tightening up the regex pattern.

Includes unit tests via the `WP.AlternativeFunctions` sniff.
@jrfnl jrfnl added Type: Bug Status: Review ready Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native labels Feb 7, 2020
@jrfnl jrfnl added this to the 2.x Next milestone Feb 7, 2020
@jrfnl jrfnl requested a review from GaryJones February 14, 2020 05:09
@GaryJones GaryJones merged commit 0e2a6a0 into develop Feb 14, 2020
@GaryJones GaryJones deleted the feature/alternative-functions-allow-curl-version-take-2 branch February 14, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants