Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#48265 new defect (bug)

The privacy export files cleanup can run unlink on directories throwing an error.

Reported by: garrett-eclipse's profile garrett-eclipse Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: dev-feedback
Focuses: Cc:

Description

Looking into some test failures on VVV flagged in Slack here;
https://wordpress.slack.com/archives/C02RQBWTW/p1570445063460400

It was found that the wp_privacy_delete_old_export_files function runs unlink on all files and subdirectories older than wp_privacy_export_expiration, this becomes an issue as directories can't be removed via unlink and will throw an Operation is not permitted error.

This occurs as the $export_files list is collected from a list_files call which has a level of 100 resulting in subdirectories being included.

So the question for me is should the export cleanup only do files in the current $export_dir, or should it recurse into subdirectories and if so should the directories also be cleaned up if older than wp_privacy_export_expiration. If we do go to the extent of removing subdirectories should the list_files call be updated with an exclude filter so plugins can have custom directories in that location which would be avoided during the cleanup?
Note: If subdirectories are to be removed as well we'll have to recursively traverse them and remove their contents so rmdir will work.

Along with addressing the issue in wp_privacy_delete_old_export_files the cause of the original VVV test failures should also be addressed. What lies in the privacy unit test that creates the test_contents folder but doesn't clean it up here;
https://github.com/WordPress/wordpress-develop/blob/master/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportFile.php#L244

  • simply removing this directory at the end of the test should suffice.

Change History (4)

#1 @garrett-eclipse
5 years ago

Related - #44204

  • The WP Filesystem API should probably be utilized for the cleanup here.

#2 @garrett-eclipse
4 years ago

#51111 was marked as a duplicate.

#3 @garrett-eclipse
4 years ago

Relavent from the #51111 duplicate;

Background: #39975, #40856, #50664.

When running the test suite on Windows, occasionally I get some errors:

There were 4 errors:

1) Tests_Privacy_WpPrivacyDeleteOldExportFiles::test_expired_files_should_be_deleted
unlink(S:\home\wordpress.test\develop/build/wp-content/uploads/wp-personal-data-exports/test_contents/): Is a directory

S:\home\wordpress.test\develop\build\wp-includes\functions.php:7324
S:\home\wordpress.test\develop\tests\phpunit\tests\privacy\wpPrivacyDeleteOldExportFiles.php:120

2) Tests_Privacy_WpPrivacyDeleteOldExportFiles::test_unexpired_files_should_not_be_deleted
unlink(S:\home\wordpress.test\develop/build/wp-content/uploads/wp-personal-data-exports/test_contents/): Is a directory

S:\home\wordpress.test\develop\build\wp-includes\functions.php:7324
S:\home\wordpress.test\develop\tests\phpunit\tests\privacy\wpPrivacyDeleteOldExportFiles.php:131

3) Tests_Privacy_WpPrivacyDeleteOldExportFiles::test_index_file_should_never_be_deleted
unlink(S:\home\wordpress.test\develop/build/wp-content/uploads/wp-personal-data-exports/test_contents/): Is a directory

S:\home\wordpress.test\develop\build\wp-includes\functions.php:7324
S:\home\wordpress.test\develop\tests\phpunit\tests\privacy\wpPrivacyDeleteOldExportFiles.php:142

4) Tests_Privacy_WpPrivacyDeleteOldExportFiles::test_filtered_expiration_time
unlink(S:\home\wordpress.test\develop/build/wp-content/uploads/wp-personal-data-exports/test_contents/): Is a directory

S:\home\wordpress.test\develop\build\wp-includes\functions.php:7324
S:\home\wordpress.test\develop\tests\phpunit\tests\privacy\wpPrivacyDeleteOldExportFiles.php:155

This ticket was mentioned in Slack in #hosting-community by mike. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.