Revert "Revert "fix(files): better handle directories in move()"" (#…

…5012) * Revert "Revert "fix(files): better handle directories in move() (#4970)" (#5011)" This reverts commit bd74c5f. * test: add a test case for BREAKING-530 * fix: BREAKING-530
Automattic · Nov 10, 2023 · 6d3c373 · 6d3c373
1 parent c968a66
commit 6d3c373
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 5 deletions.
diff --git a/files/class-wp-filesystem-vip.php b/files/class-wp-filesystem-vip.php
@@ -10,6 +10,7 @@
 require_once __DIR__ . '/class-api-client.php';
 
 use WP_Error;
+use WP_Filesystem_Base;
 use WP_Filesystem_Direct;
 
 class WP_Filesystem_VIP extends \WP_Filesystem_Base {
@@ -218,6 +219,10 @@ public function copy( $source, $destination, $overwrite = false, $mode = false )
 			return false;
 		}
 
+		if ( $source_transport instanceof WP_Filesystem_Direct && $destination_transport instanceof WP_Filesystem_Direct ) {
+			return $source_transport->copy( $source, $destination, $overwrite, $mode );
+		}
+
 		$destination_exists = $destination_transport->exists( $destination );
 		if ( ! $overwrite && $destination_exists ) {
 			/* translators: 1: destination file path 2: overwrite param 3: `true` boolean value */
@@ -248,13 +253,24 @@ public function copy( $source, $destination, $overwrite = false, $mode = false )
 	 * @return bool
 	 */
 	public function move( $source, $destination, $overwrite = false ) {
-		$copy_results = $this->copy( $source, $destination, $overwrite );
-		if ( false === $copy_results ) {
-			return false;
+		$source_transport      = $this->get_transport_for_path( $source );
+		$destination_transport = $this->get_transport_for_path( $destination, 'write' );
+		if ( $source_transport instanceof WP_Filesystem_Direct && $destination_transport instanceof WP_Filesystem_Direct ) {
+			return $source_transport->move( $source, $destination, $overwrite );
 		}
 
-		// We don't need to set the errors here since delete() will take care of it
-		return $this->delete( $source );
+		// WP_Filesystem_Direct::get_contents() invoked by copy() will return '' for directories; this will result in directories being copied as empty files.
+		if ( $source_transport instanceof WP_Filesystem_Base && $source_transport->is_file( $source ) ) {
+			$copy_results = $this->copy( $source, $destination, $overwrite );
+			if ( false === $copy_results ) {
+				return false;
+			}
+
+			// We don't need to set the errors here since delete() will take care of it
+			return $this->delete( $source );
+		}
+
+		return false;
 	}
 
 	/**

diff --git a/tests/files/test-wp-filesystem-vip.php b/tests/files/test-wp-filesystem-vip.php
@@ -6,7 +6,9 @@
 
 use Automattic\Test\Constant_Mocker;
 use ErrorException;
+use WP_Filesystem_Base;
 use WP_Filesystem_Direct;
+use WP_Filesystem_VIP;
 use WP_UnitTestCase;
 
 require_once __DIR__ . '/../../files/class-wp-filesystem-vip.php';
@@ -351,4 +353,30 @@ public function test__get_transport_for_path__non_vip_go_env() {
 		$lang_install_result = $get_transport_for_path->invokeArgs( $this->filesystem, [ WP_CONTENT_DIR . '/languages/test.file', 'write' ] );
 		$this->assertEquals( $lang_install_result, $this->fs_direct_mock );
 	}
+
+	public function test_move_with_no_filesystem(): void {
+		global $wp_filesystem;
+		$save_wp_filesystem = $wp_filesystem;
+
+		try {
+			// phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited -- This is the point of the test.
+			$wp_filesystem = null;
+
+			$ok = WP_Filesystem();
+			self::assertTrue( $ok );
+
+			self::assertInstanceOf( WP_Filesystem_VIP::class, $wp_filesystem );
+			/** @var WP_Filesystem_Base $wp_filesystem */
+
+			$tmp    = get_temp_dir();
+			$source = $tmp . 'source.txt';
+			$dest   = $tmp . 'dest.txt';
+
+			$actual = $wp_filesystem->move( $source, $dest );
+			self::assertFalse( $actual );
+		} finally {
+			// phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
+			$wp_filesystem = $save_wp_filesystem;
+		}
+	}
 }