TGMPA / TGM-Plugin-Activation

TGM Plugin Activation is a PHP library that allows you to easily require or recommend plugins for your WordPress themes (and plugins). It allows your users to install, update and even automatically activate plugins in singular or bulk fashion using native WordPress classes, functions and interfaces. You can reference bundled plugins, plugins from the WordPress Plugin Repository or even plugins hosted elsewhere on the internet.
http://tgmpluginactivation.com/
GNU General Public License v2.0
1.75k stars 431 forks source link

Failing latest Theme Check #808

Open DeoThemes opened 3 years ago

DeoThemes commented 3 years ago

The TGM class can't pass the latest Theme Check when submitting on wordpress.org. Here is the message:

WARNING: WP_Filesystem was found in the file class-tgm-plugin-activation.php WP_Filesystem should only be used for theme upgrade operations, not for all file operations. Consider using file_get_contents(), scandir(), or glob()

Line 795: * Uses WP_Filesystem to process and handle the plugin installation
Line 800: * @uses WP_Filesystem
Line 831: // Pass necessary information via URL if WP_Filesystem is needed.
Line 844: $method = ''; // Leave blank so WP_Filesystem can populate it as necessary.
Line 851: if ( ! WP_Filesystem( $creds ) ) {
Line 852: request_filesystem_credentials( esc_url_raw( $url ), $method, true, false, array() ); // Setup WP_Filesystem.
Line 1003: wp_filesystem'] ) ) {
Line 1008: wp_filesystem']->dirlist( $remote_source ) );
Line 1009: wp_filesystem']->is_dir( $source ) ) {
Line 1037: wp_filesystem']->move( $from_path, $to_path ) ) {
Line 2887: * through if a user has to use WP_Filesystem to enter their credentials.
Line 2960: // Pass all necessary information if WP_Filesystem is needed.
Line 2969: $method = ''; // Leave blank so WP_Filesystem can populate it as necessary.
Line 2970: $fields = array_keys( $_POST ); // Extra fields to pass to WP_Filesystem.
Line 2977: // Now we have some credentials, setup WP_Filesystem.
Line 2978: if ( ! WP_Filesystem( $creds ) ) {
jrfnl commented 3 years ago

@DeoThemes Sounds more like an issue which should be reported to Theme Check as TGMPA is using the WP_Filesystem for upgrade operations.

carolinan commented 3 years ago

This check is a warning, not an error or requirement. It is a "heads up, you need to look through the documentation for this function, learn what its purpose is, and then decide if this particular code needs to be changed or not".

The themes team has no objections to TGMPA using wp_filesystem, the themes team has no objections to using wp_filesystem if there is a clear, legit use case, and no better option available.

TGMPA files are not being excluded from the checks, because that would be a security issue, since it would be too easy to bypass theme check that way.

jrfnl commented 3 years ago

Thanks @carolinan for clearing that up. I've seen more people getting confused about this. It may be good to let people know that TGMPA is actually using things the right way.

carolinan commented 3 years ago

It is the theme authors responsibility to know what code they place in their theme, even third party code.

There is no intention to let the theme check plugin confirm that all TGMPA files are intact, or confirm that the files have not been changed or updated incorrectly.

carolinan commented 3 years ago

I still think it would be better to include this kind of functionality in WP Core, so that the theme authors would not need to add their own solution, since doing that adds to complexity and risk.

jrfnl commented 3 years ago

It is the theme authors responsibility to know what code they place in their theme, even third party code.

I agree, but we also live in a reality.

I still think it would be better to include this kind of functionality in WP Core, so that the theme authors would not need to add their own solution, since doing that adds to complexity and risk.

It would most definitely be nice to have this kind of functionality in WP Core and I've been advocating as much for years (see: talk about this - slides).

All the same, even if it would be implemented in Core, that still probably won't take away the need for external tooling like this as any implementation in Core will have limits to it, like "updates to be retrieved from the wp.org" repo. So for updates from a zip file for purchased add-ons, or updates from an external service, like GitHub, extra logic would need to be added.

adds to complexity and risk.

As for complexity and risk, there is no difference between a well-build and secure external solution and a WP Core solution. WP Core will always add hooks in strategic places to allow for extending the functionality offered, so that would no more or less secure than the current situation.