Remove ~~Review~~ the WordPressVIPMinimum.Functions.DynamicCalls sniff #517

Activity

1. GaryJones

   assigned

   jrfnl

   on Jul 27, 2020
2. jrfnl commented on Oct 23, 2020

   

   jrfnl

   on Oct 23, 2020

   CollaboratorAuthor

   More actions

   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.
3. jrfnl commented on Jul 27, 2025

   

   jrfnl

   on Jul 27, 2025

   Last edited by jrfnl

   CollaboratorAuthor

   More actions

   I've had another look at this and my conclusion is the same: taking variable scoping into account is going to be very very hard.

   • Function parameters would need to be "scoped" to the function (file + scope opener + scope closer) for which they are declared.
   • Function local variables would need to be "scoped" to the function within which they are declared.
   • However, then we would also need to content with global ... statements importing variables from the global space.
   • As well as the use of $GLOBALS['var_name'].
   • Of course, this would also need to take into account that functions can be nested and variables declared in the outer function are not accessible within the inner function.
   • For closures, we'd need to take variables imported via use clauses into account.
   • And then there are arrow functions which have access to variables outside their own scope...
   • Next, we'd need to take properties declared in classes into account and map those to the use of $this->prop_name within the class.
   • Etc etc.

   On top of that, in most cases, PHPCS will be run in parallel mode, where each process will have it's own fork of the sniff and the "variable cache" (stored in the $variables_arr) will not be shared across the forks/processes, which will cause false negatives for (global) variables seen in a files scanned by process A, but dynamically called from a file scanned in process B.

   Limiting the "variable cache" to the current file and adding scoping information will make the sniff more reliable, but will also cause false negatives for global variables defined in other files. Then again, we already have those due to the parallel processing.

   All in all, my previous conclusion stands.
4. GaryJones commented on Jul 28, 2025

   

   GaryJones

   on Jul 28, 2025

   Contributor

   More actions

   This sniff enforces that certain functions are not dynamically called.
   — sniff class description

   Dynamic calls to certain functions are not allowed in PHP 7.1 and forwards. This sniff will find some of these.

   http://php.net/manual/en/migration71.incompatible.php
   — PR that added the sniff in 2018

   Given that these dynamic calls triggered a warning in PHP 7.1+, and a fatal error in PHP 8.0, I'm good with removing this sniff from being used in VIPCS 3.1.0 and deleting it fully in 4.0.
5. jrfnl

   modified the milestones: 3.1.0, 4.0.0 on Jul 28, 2025
6. jrfnl

   changed the title [-]Review the WordPressVIPMinimum.Functions.DynamicCalls sniff[/-] [+]Remove ~~Review~~ the WordPressVIPMinimum.Functions.DynamicCalls sniff[/+] on Jul 28, 2025
7. jrfnl

   removed their assignment

   on Jul 28, 2025