AlternativeFunctions: improve handling of file_get_contents() #1374
Conversation
The `AlternativeFunctions` sniff was throwing two different warnings whenever `file_get_contents()` was encountered, one advising to use `wp_remote_get()`, one advising to use the `WP_FileSystem`. Aside from that, the warning is only really relevant when retrieving a remote URL. Using `file_get_contents()` for local files should not trigger a warning as discussed in 943. This PR eliminates the notice about using the `WP_FileSystem` in favour of `wp_remote_get()`. It also adds a parameter check to attempt to determine whether a local or a remote file is being retrieved and eliminates the `warning` when it is clear that `file_get_contents()` is used only to retrieve a local file. Includes unit tests. Fixes 943
| break; | ||
| } | ||
|
|
||
| if ( preg_match( '`\b(?:ABSPATH|WP_(?:CONTENT|PLUGIN)_DIR|WPMU_PLUGIN_DIR|TEMPLATEPATH|STYLESHEETPATH|(?:MU)?PLUGINDIR)\b`', $params[1]['raw'] ) === 1 ) { |
There was a problem hiding this comment.
Feels like these regexes could be centralised for potential re-use, or the whole preg_match() could be inside a named local / parent sniff method to become a little more self-documenting.
if ( $this->matchesConstantsThatIndicateALocalFile( $params[1]['raw'] ) {
return;
}
if ( $this->matchesFunctionsThatIndicateALocalFile( $params[1]['raw'] ) {
return;
}There was a problem hiding this comment.
@GaryJones Good question.
IIRC, so far, we haven't needed anything like it elsewhere. Generally speaking, I'd move them to be more re-usable once a secondary use-case has been identified.
All the same, I'm not fussed about this, so what about adding them as class constants to the Sniff class and using the x modifier to spread them over several lines with documentation ?
Moving the whole function call to a boolean return function feels wrong as that would prevent sniffs which use the regexes from getting access to the $matches if so desired.
There was a problem hiding this comment.
Could the new method return the matches, and then the use case here cast to a bool if needed?
There was a problem hiding this comment.
Could the new method return the matches, and then the use case here cast to a bool if needed?
The thing is preg_match() has two results:
- The function return value:
1for a match,0for no match orfalsefor an error/fault regex etc - The
$matchesarray which may or may not be filled with matches.
By returning just the $matches, we would loose the nuances the function return value provides.
I'm wondering if we're not trying to create a solution for a non-issue. Have you got a (future) sniff in mind which would need these regex(es) or an existing one which could benefit from using them ?
If not, let's leave the abstraction until the time when we do have a second use-case as that might also clarify what the best way to abstract this would be.
|
@JDGrimes Just checking - have you got time to have a look at this PR in the near future ? Or was there a specific reason not to approve this PR (yet) ? |
JDGrimes
deleted the
feature/943-alternativefunction-file_get_contents-handling
branch
The
AlternativeFunctionssniff was throwing two different warnings wheneverfile_get_contents()was encountered, one advising to usewp_remote_get(), one advising to use theWP_FileSystem.Aside from that, the warning is only really relevant when retrieving a remote URL. Using
file_get_contents()for local files should not trigger a warning as discussed in #943.This PR eliminates the notice about using the
WP_FileSystemin favour ofwp_remote_get().It also adds a parameter check to attempt to determine whether a local or a remote file is being retrieved and eliminates the
warningwhen it is clear thatfile_get_contents()is used only to retrieve a local file.Includes unit tests.
Fixes #943