Skip to content

Commit

Permalink
PHPCS: Clean up some WordPress.WP.AlternativeFunctions sniffs (Automa…
Browse files Browse the repository at this point in the history
…ttic#28198)

In packages/autoloader we can just ignore the sniff entirely. None of
the code there runs inside WordPress, so we don't need to use
WordPress's alternative functions.

The same goes for the tests in packages/my-jetpack.

The `Helper_Script_Manager` classes in packages/backup and
packages/transport-helper, on the other hand, can make more consistent
use of WordPress's `WP_Filesystem` class, instead of mixing it with
direct filesystem access.
  • Loading branch information
anomiex authored Jan 9, 2023
1 parent 16a66db commit d96f8b7
Show file tree
Hide file tree
Showing 17 changed files with 185 additions and 89 deletions.
9 changes: 9 additions & 0 deletions projects/packages/autoloader/.phpcs.dir.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?xml version="1.0"?>
<ruleset>

<!-- This doesn't run inside of WordPress, do we don't need to use WordPress's alternative functions. -->
<rule ref="WordPress.WP.AlternativeFunctions">
<severity>0</severity>
</rule>

</ruleset>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: changed
Comment: Globally ignore WordPress.WP.AlternativeFunctions sniff, no code here runs inside of WordPress so we'll never want those functions.


3 changes: 0 additions & 3 deletions projects/packages/autoloader/src/AutoloadFileWriter.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@
// phpcs:disable WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase
// phpcs:disable WordPress.NamingConventions.ValidVariableName.PropertyNotSnakeCase
// phpcs:disable WordPress.PHP.DevelopmentFunctions.error_log_var_export
// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents
// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_fopen
// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_fwrite

namespace Automattic\Jetpack\Autoloader;

Expand Down
3 changes: 0 additions & 3 deletions projects/packages/autoloader/src/AutoloadGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@
// phpcs:disable PHPCompatibility.Keywords.NewKeywords.t_dirFound
// phpcs:disable WordPress.Files.FileName.InvalidClassFileName
// phpcs:disable WordPress.PHP.DevelopmentFunctions.error_log_var_export
// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents
// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_fopen
// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_fwrite
// phpcs:disable WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid
// phpcs:disable WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase
// phpcs:disable WordPress.NamingConventions.ValidVariableName.InterpolatedVariableNotSnakeCase
Expand Down
2 changes: 0 additions & 2 deletions projects/packages/autoloader/src/CustomAutoloaderPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ private function determineSuffix() {

// Reuse our own suffix, if any.
if ( is_readable( $vendorPath . '/autoload_packages.php' ) ) {
// phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
$content = file_get_contents( $vendorPath . '/autoload_packages.php' );
if ( preg_match( '/^namespace Automattic\\\\Jetpack\\\\Autoloader\\\\jp([^;\s]+);/m', $content, $match ) ) {
return $match[1];
Expand All @@ -153,7 +152,6 @@ private function determineSuffix() {

// Reuse Composer's suffix, if any.
if ( is_readable( $vendorPath . '/autoload.php' ) ) {
// phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
$content = file_get_contents( $vendorPath . '/autoload.php' );
if ( preg_match( '{ComposerAutoloaderInit([^:\s]+)::}', $content, $match ) ) {
return $match[1];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function get_coverage_version() {
*/
function count_lines_before_class_keyword( $file ) {
// Find the line that the `class` keyword occurs on so that we can use it to calculate an offset from the header.
$content = file_get_contents( $file ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
$content = file_get_contents( $file );

// Find the class keyword and capture the number of characters in the string before this point.
if ( 0 === preg_match_all( '/^class /m', $content, $matches, PREG_OFFSET_CAPTURE ) ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
* @package automattic/jetpack-autoloader
*/

// phpcs:disable WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents
// phpcs:disable WordPress.WP.AlternativeFunctions.json_encode_json_encode
// phpcs:disable WordPress.PHP.DiscouragedPHPFunctions.system_calls_exec

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ function set_test_is_multisite( $is_multisite ) {
* @return false|string The JSON encoded string, or false if it cannot be encoded.
*/
function wp_json_encode( $data, $options = 0, $depth = 512 ) {
// phpcs:ignore WordPress.WP.AlternativeFunctions.json_encode_json_encode
return json_encode( $data, $options, $depth );
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
// We live in the namespace of the test autoloader to avoid many use statements.
namespace Automattic\Jetpack\Autoloader\jpCurrent;

// phpcs:disable WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents

use Automattic\Jetpack\Autoloader\ManifestGenerator;
use PHPUnit\Framework\TestCase;

Expand Down
4 changes: 4 additions & 0 deletions projects/packages/backup/changelog/update-wpcs-pre-phpcbf-3
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Use `WP_Filesystem` more consistently in `Helper_Script_Manager`.
109 changes: 73 additions & 36 deletions projects/packages/backup/src/class-helper-script-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public static function install_helper_script( $script_body ) {
// Replace '[wp_path]' in the Helper Script with the WordPress installation location. Allows the Helper Script to find WordPress.
$script_body = str_replace( '[wp_path]', addslashes( ABSPATH ), $script_body );

$wp_filesystem = self::get_wp_filesystem();
if ( ! $wp_filesystem ) {
return new \WP_Error( 'install_failed', 'Failed to install Helper Script' );
}

// Create a jetpack-temp directory for the Helper Script.
$temp_directory = self::create_temp_directory();
if ( \is_wp_error( $temp_directory ) ) {
Expand All @@ -63,11 +68,11 @@ public static function install_helper_script( $script_body ) {
$file_name = 'jp-helper-' . $file_key . '.php';
$file_path = trailingslashit( $temp_directory['path'] ) . $file_name;

if ( ! file_exists( $file_path ) ) {
if ( ! $wp_filesystem->exists( $file_path ) ) {
// Attempt to write helper script.
if ( ! self::put_contents( $file_path, $script_body ) ) {
if ( file_exists( $file_path ) ) {
unlink( $file_path );
if ( $wp_filesystem->exists( $file_path ) ) {
$wp_filesystem->delete( $file_path );
}

continue;
Expand Down Expand Up @@ -97,7 +102,12 @@ public static function install_helper_script( $script_body ) {
* @return boolean True if the file is deleted (or does not exist).
*/
public static function delete_helper_script( $path ) {
if ( ! file_exists( $path ) ) {
$wp_filesystem = self::get_wp_filesystem();
if ( ! $wp_filesystem ) {
return false;
}

if ( ! $wp_filesystem->exists( $path ) ) {
return true;
}

Expand All @@ -106,7 +116,7 @@ public static function delete_helper_script( $path ) {
return false;
}

return unlink( $path );
return $wp_filesystem->delete( $path );
}

/**
Expand Down Expand Up @@ -139,16 +149,21 @@ public static function delete_all_helper_scripts() {
* @param int|null $expiry_time If specified, only delete scripts older than $expiry_time.
*/
public static function cleanup_helper_scripts( $expiry_time = null ) {
$wp_filesystem = self::get_wp_filesystem();
if ( ! $wp_filesystem ) {
return;
}

foreach ( self::get_install_locations() as $directory => $url ) {
$temp_dir = trailingslashit( $directory ) . self::TEMP_DIRECTORY;

if ( is_dir( $temp_dir ) ) {
if ( $wp_filesystem->is_dir( $temp_dir ) ) {
// Find expired helper scripts and delete them.
$helper_scripts = glob( trailingslashit( $temp_dir ) . 'jp-helper-*.php' );
$helper_scripts = $wp_filesystem->dirlist( $temp_dir );
if ( is_array( $helper_scripts ) ) {
foreach ( $helper_scripts as $filename ) {
if ( null === $expiry_time || filemtime( $filename ) < $expiry_time ) {
self::delete_helper_script( $filename );
foreach ( $helper_scripts as $entry ) {
if ( preg_match( '/^jp-helper-*\.php$/', $entry['name'] ) && ( null === $expiry_time || $entry['lastmodunix'] < $expiry_time ) ) {
self::delete_helper_script( trailingslashit( $temp_dir ) . $entry['name'] );
}
}
}
Expand All @@ -169,14 +184,18 @@ public static function cleanup_helper_scripts( $expiry_time = null ) {
* @return boolean True if the directory is deleted
*/
private static function delete_empty_helper_directory( $dir ) {
if ( ! is_dir( $dir ) ) {
$wp_filesystem = self::get_wp_filesystem();
if ( ! $wp_filesystem ) {
return false;
}

if ( ! $wp_filesystem->is_dir( $dir ) ) {
return false;
}

// Tally the files in the target directory, and reject if there are too many.
$glob_path = trailingslashit( $dir ) . '*';
$dir_contents = glob( $glob_path );
if ( count( $dir_contents ) > 2 ) {
$dir_contents = $wp_filesystem->dirlist( $dir );
if ( $dir_contents === false || count( $dir_contents ) > 2 ) {
return false;
}

Expand All @@ -186,8 +205,9 @@ private static function delete_empty_helper_directory( $dir ) {
'index.php' => self::INDEX_FILE,
);

foreach ( $dir_contents as $path ) {
$basename = basename( $path );
foreach ( $dir_contents as $entry ) {
$basename = $entry['name'];
$path = trailingslashit( $dir ) . $basename;
if ( ! isset( $allowed_files[ $basename ] ) ) {
return false;
}
Expand All @@ -197,14 +217,15 @@ private static function delete_empty_helper_directory( $dir ) {
return false;
}

if ( ! unlink( $path ) ) {
if ( ! $wp_filesystem->delete( $path ) ) {
return false;
}
}

// If the directory is now empty, delete it.
if ( count( glob( $glob_path ) ) === 0 ) {
return rmdir( $dir );
$dir_contents = $wp_filesystem->dirlist( $dir );
if ( $dir_contents === false || count( $dir_contents ) === 0 ) {
return $wp_filesystem->rmdir( $dir );
}

return false;
Expand All @@ -219,16 +240,21 @@ private static function delete_empty_helper_directory( $dir ) {
* @return WP_Error|array Array containing the url and path of the temp directory if successful, WP_Error if not.
*/
private static function create_temp_directory() {
$wp_filesystem = self::get_wp_filesystem();
if ( ! $wp_filesystem ) {
return new \WP_Error( 'temp_directory', 'Failed to create jetpack-temp directory' );
}

foreach ( self::get_install_locations() as $directory => $url ) {
// Check if the install location is writeable.
if ( ! is_writeable( $directory ) ) {
if ( ! $wp_filesystem->is_writable( $directory ) ) {
continue;
}

// Create if one doesn't already exist.
$temp_dir = trailingslashit( $directory ) . self::TEMP_DIRECTORY;
if ( ! is_dir( $temp_dir ) ) {
if ( ! mkdir( $temp_dir ) ) {
if ( ! $wp_filesystem->is_dir( $temp_dir ) ) {
if ( ! $wp_filesystem->mkdir( $temp_dir ) ) {
continue;
}

Expand Down Expand Up @@ -272,13 +298,8 @@ private static function write_supplementary_temp_files( $dir ) {
* @return boolean True if successfully written.
*/
private static function put_contents( $file_path, $contents ) {
global $wp_filesystem;

if ( ! function_exists( '\\WP_Filesystem' ) ) {
require_once ABSPATH . 'wp-admin/includes/file.php';
}

if ( ! \WP_Filesystem() ) {
$wp_filesystem = self::get_wp_filesystem();
if ( ! $wp_filesystem ) {
return false;
}

Expand All @@ -296,13 +317,8 @@ private static function put_contents( $file_path, $contents ) {
* @return boolean True if the file exists, is readable, and the header matches.
*/
private static function verify_file_header( $file_path, $expected_header ) {
global $wp_filesystem;

if ( ! function_exists( '\\WP_Filesystem' ) ) {
require_once ABSPATH . 'wp-admin/includes/file.php';
}

if ( ! \WP_Filesystem() ) {
$wp_filesystem = self::get_wp_filesystem();
if ( ! $wp_filesystem ) {
return false;
}

Expand Down Expand Up @@ -344,4 +360,25 @@ public static function get_install_locations() {
return $install_locations;
}

/**
* Get the WP_Filesystem.
*
* @return \WP_Filesystem|null
*/
private static function get_wp_filesystem() {
global $wp_filesystem;

if ( ! $wp_filesystem ) {
if ( ! function_exists( '\\WP_Filesystem' ) ) {
require_once ABSPATH . 'wp-admin/includes/file.php';
}

if ( ! \WP_Filesystem() ) {
return null;
}
}

return $wp_filesystem;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ public function test_install_backup_helper_script_success() {
$this->assertArrayHasKey( 'path', $response_data );

// Cleanup.
unlink( $response_data['path'] );
wp_delete_file( $response_data['path'] );
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Ignore WordPress.WP.AlternativeFunctions sniff in tests. No need to use WordPress's filesystem functions there.


9 changes: 9 additions & 0 deletions projects/packages/my-jetpack/tests/php/.phpcs.dir.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?xml version="1.0"?>
<ruleset>

<!-- We don't need to use WordPress's filesystem functions to set up tests. -->
<rule ref="WordPress.WP.AlternativeFunctions">
<severity>0</severity>
</rule>

</ruleset>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Use `WP_Filesystem` more consistently in `Helper_Script_Manager`.
Loading

0 comments on commit d96f8b7

Please sign in to comment.