Context Navigation

← Previous Ticket
Next Ticket →

#61590 closed enhancement (fixed)

Make wp_delete_file return to be compliant both with PHP and WPCS

Reported by:	anonymized_14808221	Owned by:	SergeyBiryukov
Priority:	normal	Milestone:	6.7
Component:	Filesystem API	Version:	6.6
Severity:	normal	Keywords:	2nd-opinion has-patch
Cc:		Focuses:

Description

When you use unlink() you can check if it was successful via the return value of unlink().
But WPCS expects us to use wp_delete_file() which does not return, making it impossible to check if it was successful, without doing overhead checks if the file still exists.

I suggest returning the value of unlink in wp_delete_file so we can actually comply with WPCS while still using PHP features.

function wp_delete_file( $file ) {
        /**
         * Filters the path of the file to delete.
         *
         * @since 2.1.0
         *
         * @param string $file Path to the file to delete.
         */
        $delete = apply_filters( 'wp_delete_file', $file );
        if ( ! empty( $delete ) ) {
                return @unlink( $delete );
        }
}

Change History (5)

#1 @anonymized_14808221
2 years ago

.. and probably also better error handling for when $delete is empty by simply removing the check for empty. It should return false in those cases.

function wp_delete_file( $file ) {
        /**
         * Filters the path of the file to delete.
         *
         * @since 2.1.0
         *
         * @param string $file Path to the file to delete.
         */
        $delete = apply_filters( 'wp_delete_file', $file );
        return @unlink( $delete );
}

This ticket was mentioned in PR #6993 on WordPress/wordpress-develop by @debarghyabanerjee.

2 years ago #2

Keywords has-patch added

Trac Ticket - Core-61590

## Summary

This Pull Request addresses the issue where using wp_delete_file() lacks a return value, making it challenging to verify the success of file deletion operations compared to unlink().

## Description

### Problem

The wp_delete_file() lacks a return value, making it challenging to verify the success of file deletion operations without doing overhead checks if the file still exists.

### Example

wp_delete_file( $file ) => returns void

### Solution

Since the wp_delete_file() internally uses the unlink function which returns a boolean value based on success or failure, to resolve this issue, the return value of unlink() is returned i.e., a boolean value is return to make the user aware if the deletion was successful or not. Additionally, if the file is not found, it returns false.

#3 @SergeyBiryukov
2 years ago

Milestone Awaiting Review → 6.7
Owner set to SergeyBiryukov
Status new → reviewing

#4 @SergeyBiryukov
2 years ago

Resolution → fixed
Status reviewing → closed

In 58715:

Filesystem API: Add a return value for wp_delete_file().

This addresses a discrepancy where using unlink() allows for checking if it was successful via the return value, but wp_delete_file() did not have a return value, making it impossible to verify the result without doing overhead checks if the file still exists.

This also brings more consistency with the other wp_delete_*() functions, specifically:

wp_delete_file_from_directory()
wp_delete_post()
wp_delete_post_revision()
wp_delete_attachment()
wp_delete_attachment_files()
wp_delete_comment()
wp_delete_nav_menu()
wp_delete_term()
wp_delete_site()
wp_delete_user()

Includes adding basic unit tests for wp_delete_file().

Follow-up to [31575].

Props bedas, debarghyabanerjee, mukesh27, SergeyBiryukov.
Fixes #61590.

@debarghyabanerjee commented on PR #6993:

2 years ago #5

Closing the PR since the issue has already been resolved.

Note: See TracTickets for help on using tickets.

Download in other formats:

Make WordPress Core

Context Navigation

#61590 closed enhancement (fixed)

Make wp_delete_file return to be compliant both with PHP and WPCS

Description

Change History (5)

#1 @anonymized_14808221 2 years ago

This ticket was mentioned in ​PR #6993 on ​WordPress/wordpress-develop by ​@debarghyabanerjee. 2 years ago #2

#3 @SergeyBiryukov 2 years ago

#4 @SergeyBiryukov 2 years ago

​@debarghyabanerjee commented on ​PR #6993: 2 years ago #5