[Bug]: file_get_contents in LibraryProperties triggers WP Plugin Check
Description of the bug
This is a "bug" more in the way of an "annoyance" rather than a problem with the code.
When a plugin is tested against the official Plugin Check utility, one of the warnings/errors will be that we're supposed to use wp_remote_get instead of file_get_contents. This is clearly a false-positive since we are not making an HTTP call.
There has been an attempt to make the behaviour of the sniff a little smarter to check for a few obvious "local usage scenarios", but our usage here still causes the warning.
It seems we have 2 options:
- Try to trigger the "local usage detection" somehow. Judging from the code, the only real option is to pass
$use_include_path = true, but I am not sure we want that - Accompany the line with a comment like
// phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents -- Reading local file is OK.in order to disable the sniff. The actual plugin review process may or may not ignore comments like these. But it would at least put up a strong argument that we are aware of the situation and that we are facing a false-positive here.
cc @mmaymo
Reproduction instructions
- Download & install a package that uses modularity, for example Mollie which made us aware of the issue
- Download & install Plugin Check
- Execute tests (
wp plugin check <PLUGIN_FILE>.php)
Expected behavior
In a perfect world, we would see no complaints originating from modularity
Environment info
No response
Relevant log output
FILE: lib/packages/Inpsyde/Modularity/Properties/LibraryProperties.php 43 29 ERROR WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
Additional context
No response
Code of Conduct
- [X] I agree to follow this project's Code of Conduct
I don't consider this a bug.
Every project is expected to comply with the coding standards it commits to, and Modularity is not committed to WordPress coding standards, nor it is committed to "Plugin Check".
This library respects the coding standards it committs to, so there's no unexpected behavior, hence there's no bug.
At the current state, I see this as a problem of the specific project (Mollie, in your example) that decided to use this library and also to be compliant with "Plugin Check".
So normally this is something I would expect to be solved at that project level, but honestly, I'm not sure this is possible. For sure, it is possible to have in that plugin a PHPCS configuration like:
<rule ref="WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents">
<exclude-pattern>*/LibraryProperties.php</exclude-pattern>
</rule>
But I have no idea if "Plugin Check" would respect the custom PHPCS configuration in the plugin.
We could commit this library to "Plugin Check" compatibility, but that creates a coupling that could be problematic. Even because we should consider doing the same in other libraries, and possibly checking automatically.
So, to sum it up, I see 2 options:
- We don't commit to respect Plugin Check, so there's nothing to do here.
- We do commit to Plugin Check. But that's a feature request (not a bug) that deserves to be evaluated in a wider context, considerng impact to this and other libraries, look for automatic checks, etc.
Limiting the issue to this very narrow scope of this very specific error report, and so adding a single inline comment to bypass a styling rule the project is not committed to... is not the way I see it done.