inpsyde / modularity

A PSR-11 implementation for WordPress Plugins, Themes or Libraries.
https://inpsyde.github.io/modularity/
GNU General Public License v2.0
46 stars 4 forks source link

[Bug]: file_get_contents in LibraryProperties triggers WP Plugin Check #50

Closed Biont closed 1 month ago

Biont commented 3 months ago

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:

cc @mmaymo

Reproduction instructions

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

gmazzap commented 3 months ago

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:

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.