WordPress.org

Make WordPress Core

Opened 11 days ago

Last modified 10 days ago

#48265 new defect (bug)

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

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

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 (1)

#1 @garrett-eclipse
10 days ago

Related - #44204

  • The WP Filesystem API should probably be utilized for the cleanup here.
Note: See TracTickets for help on using tickets.