Review the WordPressVIPMinimum.Functions.DynamicCalls sniff

[jrfnl]

Review the WordPressVIPMinimum.Functions.DynamicCalls sniff for the following in as far as relevant to that sniff:

• Code style independent sniffing / Correct handling of quirky code
  Typical things to add tests for and verify correct handling of:
  • Nested function/closure declarations
  • Nested class declarations
  • Comments in unexpected places
  • Variables being assigned to via list statements
  • Multiline text strings
  • Text strings provided via heredoc/nowdoc
  • Use of short open tags
  • Using PHP close tag as end of statement
  • Inline control structures (without braces)
• Code simplifications which can be made using PHPCSUtils
• Sniff stability improvements which can be made using PHPCSUtils
• Correct handling of modern PHP code
  Typical things to add tests for and verify correct handling of (where applicable):
  • PHP 5.0 Try/catch/finally (PHP 5.5) and exceptions
  • PHP 5.3 Namespaced code vs code in the global namespace
  • PHP 5.3 Use import statements, incl aliasing
  • PHP 5.3 Short ternaries
  • PHP 5.3 Closures, incl closure use
  • PHP 5.4 Short arrays
  • PHP 5.5 Class name resolution using ::class
  • PHP 5.5 List in foreach
  • PHP 5.5/7.0 Generators using yield and yield from
  • PHP 5.6 Constant scalar expressions
  • PHP 5.6 Importing via use function/const
  • PHP 7.0 Null coalesce
  • PHP 7.0 Anonymous classes
  • PHP 7.0 Scalar and return type declarations
  • PHP 7.0 Group use statements
  • PHP 7.1 Short lists
  • PHP 7.1 Keyed lists
  • PHP 7.1 Multi-catch
  • PHP 7.1 Nullable types
  • PHP 7.3 List reference assignments
  • PHP 7.4 arrow functions
  • PHP 7.4 numeric literals with underscores
  • PHP 7.4 null coalesce equals
  • PHP 7.4 Typed properties
  • Various versions: trailing comma's in function calls, group use, function declarations, closure use etc

Other:

• Review violation error vs warning
• Review violation severity
• Review violation message, consider adding a link
• Check open issues related to the sniff
• Review PHPDoc comments

Sniff basics, but changes need to be lined up for next major release:

• Inappropriate use of public properties (Audit Public Sniff Properties #234)
• Modular error codes (unique error code for each distinct message)

Once PHPCS/PHPCSUtils supports this:

• PHP 8.0 Constructor property promotion
• PHP 8.0 Union types
• PHP 8.0 match expressions
• PHP 8.0 Nullsafe operator
• PHP 8.0 Named arguments
• PHP 8.0 Single token namespaced names

[jrfnl]

Instigated by the bug report in #590, I have done a review of this sniff and can only conclude that this sniff is fundamentally flawed.

The sniff looks at variables, but does not take variable scope into account.

Code example demonstrating the issue:

function foo() {
    $var = 'extract';
}

function bar( callable $var ) {
    $var(); // <= The sniff would throw an error here for dynamically calling `extract()`.
}

While the simple example above demonstrates the problem, the actual problem is much larger as the sniff also doesn't keep track of file changes, so a variable declared in file A, could lead to a false positive for a dynamic function call in file H.

The cache will also overwrite previously identified variables without taking context into account, compounding the issue.

While PR #592 will get rid of the immediate issues in the sniff, it does not address this fundamental flaw.

Limited check

As annotated in the sniff, it only looks for a select syntax to detect the issue, ignoring the most common problem case of the function names being used in callbacks.

Conclusion

Fixing the flaws in this sniff would not be an easy fix and basically requires writing a completely new sniff.

As there are no significant bug reports about this, I'm going to suggest leaving it as is for now.

If, at some point, bug reports start coming in for the identified issues, I suggest removing the sniff altogether.
If, at some point, the PHPCompatibility standard would add a (better) sniff for this, I suggest switching over or removing the sniff here and recommending people use the PHPCompatibility standard for these type of issues.

Note: the PHPCompatibility standard does not detect this issue at this time exactly because it is not easy to get this right.