From 680aa31f0c70d7f013986e0efa755353a5dcf4f2 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 21 Jul 2025 01:25:42 +0200 Subject: [PATCH 1/6] Performance/FetchingRemoteData: extend AbstractFunctionParameterSniff As things were, the determination of whether or not a `T_STRING` is a call to the global PHP native `str_replace()` function was severely flawed. By switching the sniff over to be based on the WordPressCS `AbstractFunctionParameterSniff` class, this flaw is mitigated. Includes adding a slew of additional tests, some of which (line 8 - 13, 28, 37, 45) are specific to the flaw being addressed in this commit. Additionally, the tests have been made more comprehensive and varied by: * Testing against false positives for calls to methods or namespaced function calls (= the issue being addressed in this PR). * Testing against false positives for attribute class using the same name as the function. * Ensure function import `use` statements are not flagged. We're not interested in those. * Adding more variations to the pre-existing tests: - Non-lowercase function call(s). - Fully qualified function calls. - Use PHP 7.3+ trailing comma's in a few function calls. - Use both single quoted as well as double quoted text strings in the tests. - Use other non-plain-text tokens in the passed parameter to test the `FileGetContentsUnknown` warning, which was previously untested. - Multi-line function call. - Comments in function call. --- .../Performance/FetchingRemoteDataSniff.php | 47 ++++++++++++------- .../FetchingRemoteDataUnitTest.inc | 45 ++++++++++++++++-- .../FetchingRemoteDataUnitTest.php | 8 +++- 3 files changed, 79 insertions(+), 21 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php index 358ff2ca..4db4f752 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php @@ -10,45 +10,58 @@ namespace WordPressVIPMinimum\Sniffs\Performance; use PHP_CodeSniffer\Util\Tokens; -use WordPressVIPMinimum\Sniffs\Sniff; +use PHPCSUtils\Utils\PassedParameters; +use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** * Restricts usage of file_get_contents(). */ -class FetchingRemoteDataSniff extends Sniff { +class FetchingRemoteDataSniff extends AbstractFunctionParameterSniff { /** - * Returns an array of tokens this test wants to listen for. + * The group name for this group of functions. * - * @return array + * @var string */ - public function register() { - return [ T_STRING ]; - } + protected $group_name = 'file_get_contents'; /** - * Process this test when one of its tokens is encountered. + * Functions this sniff is looking for. * - * @param int $stackPtr The position of the current token in the stack passed in $tokens. + * @var array Key is the function name, value irrelevant. + */ + protected $target_functions = [ + 'file_get_contents' => true, + ]; + + /** + * Process the parameters of a matched function. + * + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched + * in lowercase. + * @param array $parameters Array with information about the parameters. * * @return void */ - public function process_token( $stackPtr ) { - - $functionName = $this->tokens[ $stackPtr ]['content']; - if ( $functionName !== 'file_get_contents' ) { + public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { + $filename_param = PassedParameters::getParameterFromStack( $parameters, 1, 'filename' ); + if ( $filename_param === false ) { + // Missing required parameter. Probably live coding, nothing to examine (yet). Bow out. return; } - $data = [ $this->tokens[ $stackPtr ]['content'] ]; + $data = [ $matched_content ]; - $fileNameStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $stackPtr + 1, null, false, null, true ); - if ( $fileNameStackPtr === false ) { + $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $filename_param['start'], ( $filename_param['end'] + 1 ) ); + if ( $has_text_string === false ) { $message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead. If it\'s for a local file please use WP_Filesystem instead.'; $this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data ); + return; } - $fileName = $this->tokens[ $fileNameStackPtr ]['content']; + $fileName = $this->tokens[ $has_text_string ]['content']; $isRemoteFile = ( strpos( $fileName, '://' ) !== false ); if ( $isRemoteFile === true ) { diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc index 9edbd772..e2957f79 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc @@ -1,7 +1,46 @@ file_get_contents($url); +$this?->file_get_contents($file); +MyClass::file_get_contents($file); +echo FILE_GET_CONTENTS; +namespace\file_get_contents($url); -$external_resource = file_get_contents( 'https://example.com' ); // NOK. \ No newline at end of file + +/* + * These should all be okay. + */ +$file_content = file_get_contents( 'my-file.css' ); +$file_content = \File_Get_Contents( /*comment*/ 'my-file.svg', ); +$file_content = file_get_contents( __DIR__ . "/filename.css" ); + +// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute. +#[File_Get_Contents('text')] +function foo() {} + +// Incomplete function call, should be ignored by the sniff. +$live_coding = file_get_contents(); + + +/* + * These should all be flagged with a warning. + */ +// Warning FileGetContentsRemoteFile. +$external_resource = file_get_contents( 'https://example.com', ); +\file_get_contents( "http://$url" ); +$external_resource = FILE_GET_CONTENTS( + // Sort of protocol relative URL (the ":" is superfluous). + '://example.com' +); +$external_resource = file_get_contents( 'HTTP://example.com', ); + +// Warning FileGetContentsUnknown. +file_get_contents( $url, true, $context, $offset, $length ); +\File_GET_Contents( $obj->get_fileName() ); +file_get_contents( MyClass::FILE_NAME, ); diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php index 05c27d4d..428ca980 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php @@ -32,7 +32,13 @@ public function getErrorList() { */ public function getWarningList() { return [ - 7 => 1, + 35 => 1, + 36 => 1, + 37 => 1, + 41 => 1, + 44 => 1, + 45 => 1, + 46 => 1, ]; } } From 4f190eb4667ecf55fdc7b242f5b888ee81807212 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 21 Jul 2025 01:35:54 +0200 Subject: [PATCH 2/6] Performance/FetchingRemoteData: add tests for PHP 5.6 arg unpacking and PHP 8.0 named args Both are already handled correctly after the switch to the WordPressCS `AbstractFunctionParameterSniff` class. --- .../FetchingRemoteDataUnitTest.inc | 31 +++++++++++++++++++ .../FetchingRemoteDataUnitTest.php | 3 ++ 2 files changed, 34 insertions(+) diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc index e2957f79..e42ce125 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc @@ -44,3 +44,34 @@ $external_resource = file_get_contents( 'HTTP://example.com', ); file_get_contents( $url, true, $context, $offset, $length ); \File_GET_Contents( $obj->get_fileName() ); file_get_contents( MyClass::FILE_NAME, ); + +file_get_contents(...$params); // PHP 5.6 argument unpacking. Warning FileGetContentsUnknown. + +// Safeguard correct handling of function calls using PHP 8.0+ named parameters. +file_get_contents(context: $context, ); // OK, well, not really, missing required $filename param, but that's not the concern of this sniff. +file_get_contents(file: 'https://example.com',); // OK, well, not really, typo in param name, but that's not the concern of the sniff. +file_get_contents( + context: stream_context_create( + options: [ + $proxy => 'tcp://proxy.example.com:5100', + ], + ), + filename: 'my-file.svg', +); // OK. + +file_get_contents( + context: stream_context_create( + options: [ + $cafile => __DIR__ . '/cacert.pem', + ], + ), + filename: 'https://example.com' +); // Warning FileGetContentsRemoteFile. +file_get_contents( + context: stream_context_create( + options: [ + $cafile => __DIR__ . '/cacert.pem', + ], + ), + filename: $fileName, +); // Warning FileGetContentsUnknown. diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php index 428ca980..c7537e84 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php @@ -39,6 +39,9 @@ public function getWarningList() { 44 => 1, 45 => 1, 46 => 1, + 48 => 1, + 62 => 1, + 70 => 1, ]; } } From d9f59555ecd305a33273c57f4888b0e6dc29fc96 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 21 Jul 2025 01:52:56 +0200 Subject: [PATCH 3/6] Performance/FetchingRemoteData: add support for handling PHP 8.1 first class callables As first class callables will result in the function being called, but without us knowing the passed parameters, I believe it is appropriate to flag the use of `file_get_contents()` as a first class callable under the `FileGetContentsUnknown` warning. This commit implements this in a WPCS cross-version compatible manner. Prior to WPCS 3.2.0, first class callables would be send to the `process_no_parameters()` method. As of WPCS 3.2.0, they will be send to the dedicated `process_first_class_callable()` method. --- .../Performance/FetchingRemoteDataSniff.php | 66 ++++++++++++++++++- .../FetchingRemoteDataUnitTest.inc | 2 + .../FetchingRemoteDataUnitTest.php | 1 + 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php index 4db4f752..185c5ac6 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php @@ -56,9 +56,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $filename_param['start'], ( $filename_param['end'] + 1 ) ); if ( $has_text_string === false ) { - $message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead. If it\'s for a local file please use WP_Filesystem instead.'; - $this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data ); - return; + $this->add_contents_unknown_warning( $stackPtr, $data ); } $fileName = $this->tokens[ $has_text_string ]['content']; @@ -69,4 +67,66 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsRemoteFile', $data ); } } + + /** + * Process the function if no parameters were found. + * + * {@internal This method is only needed for handling PHP 8.1+ first class callables on WPCS < 3.2.0. + * Once the minimum supported WPCS is 3.2.0 or higher, this method should be removed.} + * + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched + * in lowercase. + * + * @return void + */ + public function process_no_parameters( $stackPtr, $group_name, $matched_content ) { + // Check if this is a first class callable. + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( $next === false + || $this->tokens[ $next ]['code'] !== T_OPEN_PARENTHESIS + || isset( $this->tokens[ $next ]['parenthesis_closer'] ) === false + ) { + // Live coding/parse error. Ignore. + return; + } + + // First class callable. + $firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $next + 1 ), null, true ); + if ( $this->tokens[ $firstNonEmpty ]['code'] === T_ELLIPSIS ) { + $secondNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $firstNonEmpty + 1 ), null, true ); + if ( $this->tokens[ $secondNonEmpty ]['code'] === T_CLOSE_PARENTHESIS ) { + $this->add_contents_unknown_warning( $stackPtr, [ $matched_content ] ); + } + } + } + + /** + * Process the function if it is used as a first class callable. + * + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched + * in lowercase. + * + * @return void + */ + public function process_first_class_callable( $stackPtr, $group_name, $matched_content ) { + $this->add_contents_unknown_warning( $stackPtr, [ $matched_content ] ); + } + + /** + * Add a warning if the function is used with unknown parameter(s) or with a $filename parameter for which + * it could not be determined if it references a local file or a remote file. + * + * @param int $stackPtr The position of the current token in the stack. + * @param array $data Data to use for string replacement in the error message. + * + * @return void + */ + private function add_contents_unknown_warning( $stackPtr, $data ) { + $message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead. If it\'s for a local file please use WP_Filesystem instead.'; + $this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data ); + } } diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc index e42ce125..9af71362 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc @@ -75,3 +75,5 @@ file_get_contents( ), filename: $fileName, ); // Warning FileGetContentsUnknown. + +array_walk($files, file_get_contents(...)); // PHP 8.1 first class callable. Warning FileGetContentsUnknown. diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php index c7537e84..a62609bc 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php @@ -42,6 +42,7 @@ public function getWarningList() { 48 => 1, 62 => 1, 70 => 1, + 79 => 1, ]; } } From 01eff5cfcd3a3164175cdf2e8252936174256a30 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 21 Jul 2025 02:12:09 +0200 Subject: [PATCH 4/6] Performance/FetchingRemoteData: bug fix - recognize use of __DIR__ as local file Unless people get really creative with ternaries and such, the use of `__DIR__` within the parameter basically means that we know they are retrieving a local file and we can silently ignore the function call. Includes test. --- .../Sniffs/Performance/FetchingRemoteDataSniff.php | 12 ++++++++++-- .../Tests/Performance/FetchingRemoteDataUnitTest.inc | 3 +++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php index 185c5ac6..54112313 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php @@ -52,9 +52,17 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p return; } - $data = [ $matched_content ]; + $data = [ $matched_content ]; + $param_start = $filename_param['start']; + $search_end = ( $filename_param['end'] + 1 ); - $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $filename_param['start'], ( $filename_param['end'] + 1 ) ); + $has_magic_dir = $this->phpcsFile->findNext( T_DIR, $param_start, $search_end ); + if ( $has_magic_dir !== false ) { + // In all likelyhood a local file (disregarding creative code). + return; + } + + $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $param_start, $search_end ); if ( $has_text_string === false ) { $this->add_contents_unknown_warning( $stackPtr, $data ); } diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc index 9af71362..60fef22f 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc @@ -77,3 +77,6 @@ file_get_contents( ); // Warning FileGetContentsUnknown. array_walk($files, file_get_contents(...)); // PHP 8.1 first class callable. Warning FileGetContentsUnknown. + +// Bug: don't throw a warning when __DIR__ is used as this indicates use with a local file. +\file_get_contents( __DIR__ . $filename ); // OK. From 11092fd5bac7e97a362eafcf51ba0947c465f7b8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 21 Jul 2025 02:25:19 +0200 Subject: [PATCH 5/6] Performance/FetchingRemoteData: bug fix - examine all text string tokens in the parameter As things were, only the first text string token in the parameter was being examined, which resulted in false negatives. Includes test. --- .../Sniffs/Performance/FetchingRemoteDataSniff.php | 11 +++++++++-- .../Tests/Performance/FetchingRemoteDataUnitTest.inc | 3 +++ .../Tests/Performance/FetchingRemoteDataUnitTest.php | 1 + 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php index 54112313..752b3882 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php @@ -67,9 +67,16 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $this->add_contents_unknown_warning( $stackPtr, $data ); } - $fileName = $this->tokens[ $has_text_string ]['content']; + $isRemoteFile = false; + while ( $has_text_string !== false ) { + if ( strpos( $this->tokens[ $has_text_string ]['content'], '://' ) !== false ) { + $isRemoteFile = true; + break; + } + + $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, ( $has_text_string + 1 ), $search_end ); + } - $isRemoteFile = ( strpos( $fileName, '://' ) !== false ); if ( $isRemoteFile === true ) { $message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.'; $this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsRemoteFile', $data ); diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc index 60fef22f..44326762 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc @@ -80,3 +80,6 @@ array_walk($files, file_get_contents(...)); // PHP 8.1 first class callable. War // Bug: don't throw a warning when __DIR__ is used as this indicates use with a local file. \file_get_contents( __DIR__ . $filename ); // OK. + +// Bug: check all text string tokens in the parameter. +$result = file_get_contents(( is_ssl() ? 'http' : 'https' ) . '://example.com'); // Warning FileGetContentsRemoteFile. diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php index a62609bc..5b9daa41 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php @@ -43,6 +43,7 @@ public function getWarningList() { 62 => 1, 70 => 1, 79 => 1, + 85 => 1, ]; } } From 7d23be50f7f12ea16754ef2c4ae7715bb3148994 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 21 Jul 2025 02:42:23 +0200 Subject: [PATCH 6/6] Performance/FetchingRemoteData: bug fix - false negative for compound parameters As things were, if the parameter did contain a text string token and that text string token did *not* contain the text "://", the sniff would stay silent, even when the parameter contained additional non-text string tokens, which means we cannot reliably determine whether it is a local file or not. This commit changes the order of the checks to _first_ check if any of the text string tokens in the parameter contain the "://" text. If so, it will throw the (more informative) `FileGetContentsRemoteFile` warning. If not, it will check if the parameter contained anything but text string related or empty tokens. If so, it will throw the (more generic) `FileGetContentsUnknown` warning. Includes test. --- .../Performance/FetchingRemoteDataSniff.php | 25 +++++++++++++------ .../FetchingRemoteDataUnitTest.inc | 3 +++ .../FetchingRemoteDataUnitTest.php | 1 + 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php index 752b3882..807a5529 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php +++ b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php @@ -62,24 +62,35 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p return; } - $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $param_start, $search_end ); - if ( $has_text_string === false ) { - $this->add_contents_unknown_warning( $stackPtr, $data ); - } - $isRemoteFile = false; - while ( $has_text_string !== false ) { + $search_start = $param_start; + // phpcs:ignore Generic.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition -- Valid usage. + while ( ( $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $search_start, $search_end ) ) !== false ) { if ( strpos( $this->tokens[ $has_text_string ]['content'], '://' ) !== false ) { $isRemoteFile = true; break; } - $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, ( $has_text_string + 1 ), $search_end ); + $search_start = ( $has_text_string + 1 ); } if ( $isRemoteFile === true ) { $message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.'; $this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsRemoteFile', $data ); + return; + } + + /* + * Okay, so we haven't been able to determine for certain this is a remote file. + * Check for tokens which would make the parameter contents dynamic. + */ + $ignore = Tokens::$emptyTokens; + $ignore += Tokens::$stringTokens; + $ignore += [ T_STRING_CONCAT => T_STRING_CONCAT ]; + + $has_non_text_string = $this->phpcsFile->findNext( $ignore, $param_start, $search_end, true ); + if ( $has_non_text_string !== false ) { + $this->add_contents_unknown_warning( $stackPtr, $data ); } } diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc index 44326762..40e48b7f 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc @@ -83,3 +83,6 @@ array_walk($files, file_get_contents(...)); // PHP 8.1 first class callable. War // Bug: check all text string tokens in the parameter. $result = file_get_contents(( is_ssl() ? 'http' : 'https' ) . '://example.com'); // Warning FileGetContentsRemoteFile. + +// Bug: don't presume if the parameter contains a text string token, that will be the only token. +\file_get_contents( $url . '/filename.css' ); // Warning FileGetContentsUnknown. diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php index 5b9daa41..670ef4c5 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php @@ -44,6 +44,7 @@ public function getWarningList() { 70 => 1, 79 => 1, 85 => 1, + 88 => 1, ]; } }