Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing latest Theme Check #808

Open
DeoThemes opened this issue Jun 20, 2021 · 6 comments
Open

Failing latest Theme Check #808

DeoThemes opened this issue Jun 20, 2021 · 6 comments

Comments

@DeoThemes
Copy link

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
Copy link
Contributor

jrfnl commented Jun 20, 2021

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

@carolinan
Copy link

carolinan commented Jun 22, 2021

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
Copy link
Contributor

jrfnl commented Jun 22, 2021

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
Copy link

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
Copy link

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
Copy link
Contributor

jrfnl commented Jun 23, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants