Add directory support to `WP_Filesystem_Direct::move()`. #3909

costdev · 2023-01-25T23:27:51Z

This PR adds directory support to WP_Filesystem_Direct::move() for consistency with the other WP_Filesystem_* classes.

This includes:

Updates to WP_Filesystem_Direct::move().
A new global function: wp_opcache_invalidate_directory(), to recursively call wp_opcache_invalidate().
A new wrapper function: move_dir(), for extender convenience and more informative failure reasons.
Tests for move_dir().
Tests for wp_opcache_invalidate_directory().

Testing instructions

Steps to test

Ensure that WP_DEBUG and WP_DEBUG_LOG are set to true.
Change this line to:

$result = move_dir( $source, $remote_destination, true );

Navigate to Dashboard > Updates and update one or more plugins/themes.
Navigate to Plugins > Installed plugins and update one or more plugins.
Navigate to Appearance > Themes and update some themes.

Expected results

All updates should complete as normal, with no errors/warnings/notices in wp-content/debug.log.

Trac ticket: https://core.trac.wordpress.org/ticket/57375

peterwilsoncc

Added a few notes inline.

src/wp-admin/includes/file.php

tests/phpunit/tests/functions/moveDir.php

src/wp-admin/includes/class-wp-filesystem-direct.php

src/wp-admin/includes/file.php

tests/phpunit/tests/functions/wpOpcacheInvalidateDirectory.php

src/wp-admin/includes/file.php

tests/phpunit/tests/functions/moveDir.php

src/wp-admin/includes/file.php

src/wp-admin/includes/class-wp-filesystem-direct.php

src/wp-admin/includes/file.php

afragen

I recently encountered an edge case where $from and $to differed only by a trailing slash. This resulted in the clearing of $to prior to rename() deleting the source. We could easily add a guard for that case as in the following.

	if ( trailingslashit( $from ) === trailingslashit( $to ) ) {
		return false;
	}

Would need to update @return also.

costdev#71

src/wp-admin/includes/file.php

SergeyBiryukov

All the feedback appears to be addressed.

With no new concerns raised, I believe this is now ready for commit?

Co-authored-by: Sergey Biryukov <[email protected]>

@afragen

…iling slash and/or case. Props @afragen

@afragen

Props @afragen, @SergeyBiryukov Co-authored-by: Andy Fragen <[email protected]>

@azaozz

Props @azaozz, @afragen, @SergeyBiryukov Co-authored-by: Andrew Ozz <[email protected]>

costdev · 2023-01-31T19:04:11Z

For clarity: The latest push was just to rebase on trunk and make some minor updates to test documentation 🙂

peterwilsoncc

A few more notes and a question.

This is in really good shape and I think it will be good for the first beta.

src/wp-admin/includes/file.php

tests/phpunit/tests/filesystem/wpOpcacheInvalidateDirectory.php

tests/phpunit/tests/filesystem/moveDir.php

src/wp-admin/includes/file.php

Co-authored-by: Sergey Biryukov <[email protected]>

felixarntz

@costdev @afragen Overall this looks very solid, though I have a few follow up questions / potential concerns. Please take a look.

src/wp-admin/includes/file.php

peterwilsoncc

LGTM, I think this is ready.

@felixarntz The 200ms delay is a necessary evil due to excessive caching in some set ups (VirtualBox been the most likely to be hit via VVV, Chassis and others). Without it, the wrong files can be affected by multiple calls to move_dir() and cause fatal errors.

I think it's the least bad solution, and even with the delay the switch to move_dir() is substantially faster in my testing. I suspect the reason WP wasn't hitting the caching issue in the past was because copy_dir() is so slow.

azaozz

Thinking this is ready too. Only the docblock updates for the other ::move() methods.

felixarntz

I think this is good to commit. We can probably also commit #3963 right after. 🎉

src/wp-admin/includes/file.php

azaozz · 2023-02-03T02:00:09Z

Committed: https://core.trac.wordpress.org/changeset/55204

costdev marked this pull request as ready for review January 25, 2023 23:37

costdev force-pushed the add_directory_support_to_wp_filesystem_direct_move branch 2 times, most recently from ccd19bd to 14cfb77 Compare January 26, 2023 00:50

azaozz mentioned this pull request Jan 26, 2023

Use filesystem classes for move_dir(). #3897

Closed

costdev force-pushed the add_directory_support_to_wp_filesystem_direct_move branch from 14cfb77 to 20a411e Compare January 26, 2023 03:03

peterwilsoncc reviewed Jan 26, 2023

View reviewed changes

afragen mentioned this pull request Jan 26, 2023

Switch copy_dir() with move_dir() in WP_Upgrader::install_package() #3911

Closed

costdev force-pushed the add_directory_support_to_wp_filesystem_direct_move branch 2 times, most recently from 0602168 to cdefe50 Compare January 26, 2023 05:12

mukeshpanchal27 reviewed Jan 26, 2023

View reviewed changes

src/wp-admin/includes/file.php Show resolved Hide resolved

SergeyBiryukov reviewed Jan 26, 2023

View reviewed changes

tests/phpunit/tests/functions/wpOpcacheInvalidateDirectory.php Outdated Show resolved Hide resolved

SergeyBiryukov reviewed Jan 26, 2023

View reviewed changes

src/wp-admin/includes/file.php Outdated Show resolved Hide resolved

SergeyBiryukov reviewed Jan 26, 2023

View reviewed changes

tests/phpunit/tests/functions/moveDir.php Outdated Show resolved Hide resolved

SergeyBiryukov reviewed Jan 26, 2023

View reviewed changes

src/wp-admin/includes/file.php Outdated Show resolved Hide resolved

SergeyBiryukov reviewed Jan 26, 2023

View reviewed changes

src/wp-admin/includes/class-wp-filesystem-direct.php Outdated Show resolved Hide resolved

SergeyBiryukov reviewed Jan 26, 2023

View reviewed changes

src/wp-admin/includes/file.php Outdated Show resolved Hide resolved

costdev force-pushed the add_directory_support_to_wp_filesystem_direct_move branch from 6ed97ca to 235bf29 Compare January 26, 2023 19:32

afragen suggested changes Jan 27, 2023

View reviewed changes

azaozz reviewed Jan 28, 2023

View reviewed changes

src/wp-admin/includes/file.php Outdated Show resolved Hide resolved

azaozz reviewed Jan 28, 2023

View reviewed changes

src/wp-admin/includes/file.php Outdated Show resolved Hide resolved

afragen reviewed Jan 29, 2023

View reviewed changes

src/wp-admin/includes/file.php Outdated Show resolved Hide resolved

costdev force-pushed the add_directory_support_to_wp_filesystem_direct_move branch from 3892132 to 5378949 Compare January 30, 2023 10:21

costdev requested review from azaozz and peterwilsoncc and removed request for azaozz and peterwilsoncc January 30, 2023 18:53

SergeyBiryukov approved these changes Jan 31, 2023

View reviewed changes

costdev added 2 commits January 31, 2023 17:16

Add directory support to WP_Filesystem_Direct::move().

2b5003f

Introduce wp_opcache_invalidate_directory().

04c5b25

costdev and others added 14 commits January 31, 2023 17:16

Simplify error code when destination already exists.

be98efa

Co-authored-by: Sergey Biryukov <[email protected]>

Future-proofing: Use a permalink for Composer.

8761f6e

Co-authored-by: Sergey Biryukov <[email protected]>

Correct spelling of Backward compatibility.

4af9abb

Co-authored-by: Sergey Biryukov <[email protected]>

Improve documentation for $overwrite.

348d200

Co-authored-by: Sergey Biryukov <[email protected]>

Swap the order of return types for $wp_filesystem_backup.

eb49d55

Co-authored-by: Sergey Biryukov <[email protected]>

Move test files, update docs and test class names.

6ee53c5

Update test error code.

da91a6d

Try removing the forced direct filesystem method.

05dd0af

add guard and early exit if source and destination only differ by tra…

693902d

…iling slash and/or case. Props @afragen

Add tests for the new guard.

a5494e5

Move the guard to the start of move_dir().

be5f8d7

Replace continue; with elseif.

8def8c2

Pass $to as the third parameter to new WP_Error.

c6031f6

Props @afragen, @SergeyBiryukov Co-authored-by: Andy Fragen <[email protected]>

Change ! is_wp_error() to true ===

9e1e625

Props @azaozz, @afragen, @SergeyBiryukov Co-authored-by: Andrew Ozz <[email protected]>

costdev force-pushed the add_directory_support_to_wp_filesystem_direct_move branch from 2f27c7f to 8aa303a Compare January 31, 2023 17:18

Update test documentation.

809f893

costdev force-pushed the add_directory_support_to_wp_filesystem_direct_move branch from 8aa303a to 809f893 Compare January 31, 2023 17:19

peterwilsoncc reviewed Feb 1, 2023

View reviewed changes

costdev added 2 commits February 1, 2023 02:45

Use DIR_TESTDATA constant in data provider.

654b627

assertIsArray() + sort() + assertSame() -> assertSameSets().

d959eb9

SergeyBiryukov reviewed Feb 1, 2023

View reviewed changes

src/wp-admin/includes/file.php Outdated Show resolved Hide resolved

Remove errant line break.

24574d7

Co-authored-by: Sergey Biryukov <[email protected]>

felixarntz reviewed Feb 1, 2023

View reviewed changes

src/wp-admin/includes/file.php Show resolved Hide resolved

src/wp-admin/includes/file.php Show resolved Hide resolved

src/wp-admin/includes/file.php Show resolved Hide resolved

peterwilsoncc approved these changes Feb 2, 2023

View reviewed changes

azaozz self-requested a review February 2, 2023 02:48

azaozz approved these changes Feb 2, 2023

View reviewed changes

felixarntz approved these changes Feb 2, 2023

View reviewed changes

src/wp-admin/includes/file.php Show resolved Hide resolved

azaozz closed this Feb 3, 2023

Add directory support to WP_Filesystem_Direct::move(). #3909

Add directory support to WP_Filesystem_Direct::move(). #3909

Uh oh!

Conversation

costdev commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing instructions

Steps to test

Expected results

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afragen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SergeyBiryukov left a comment

Choose a reason for hiding this comment

Uh oh!

costdev commented Jan 31, 2023

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

azaozz left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azaozz commented Feb 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Add directory support to `WP_Filesystem_Direct::move()`. #3909

Add directory support to `WP_Filesystem_Direct::move()`. #3909

costdev commented Jan 25, 2023 •

edited

Loading

afragen left a comment •

edited

Loading