Permanently deleted media files

[larssn]

Hey, good plugin, works well.

Quick question that may or may not be an issue:
When deleting a media file, the originally uploaded file gets left behind - only the different thumbnail variants gets deleted. Is this intended behavior?

We run a multisite setup, and would prefer that all files permanently deleted through Wordpress would get removed completely from S3.

Thanks.

Ps. I can delete the left over file through the CLI, so at least that is working, although not convenient:
wp s3-uploads rm uploads/2016/04/image.jpg --debug --url=www.our-multisite-setup.com

[larssn]

Further strangeness: I remove the wp_delete_file filter from S3_Uploads: Then the original file gets deleted, but the thumbnail variants doesn't. I can understand why they dont get deleted, but now I don't understand why the original file DOES get deleted... 🎱

[joehoyle]

Oops think I broke this in #87 whereby I fixed the removing of sizes, but that apparently broke the original

[larssn]

Where does the delete hook occur? Been looking through the code, and all I see is the rm function in the CLI class (which seems irrelevant), and the wp delete filter that you added.

[joehoyle]

WordPress deletes the file internally, and just passes it through the wp_delete_file hook first, which I tinkered with. Will get a patch up for this.

[larssn]

👍
Thanks for the help!

[thefrosty]

Will this get patched in 1.1.1 or in the 2.x release?

[nuvoPoint]

We are having the same issue. Is there a fix for this or should we make a fork?

[josephabrahams]

👍

[mathewemoore]

Is anyone working on a fix for this? I am working on a project and everything is working fine except for deleting the original file.

[MordiSacks]

I went through the whole project, for the life of me I can't understand why main file does not get deleted.

[ikappas]

Hi all,

When an attachment is deleted the function wp_delete_attachment is called that checks whether the attachment has thumbnail, intermmediate and backup images and removes them with the following code:

$thumbfile = apply_filters( 'wp_delete_file', $thumbfile );
@ unlink( path_join($uploadpath['basedir'], $thumbfile) );

From the snippet you can see that the wp_delete_file filter is applied and then the path is joined back together with the $uploadpath['basedir'] yielding in effect the original absolute s3 path.

When these iterations are completed wp_delete_attachment finally calls wp_delete_file that attempts to remove the main attachment file with the following code:

	$delete = apply_filters( 'wp_delete_file', $file );
	if ( ! empty( $delete ) ) {
		@unlink( $delete );
	}

As you can see this one requires an absolute file path and not a relative file, thus failing to delete the main attachment file.

The only way to fix this issue is to implement a check in class-s3-uploads::wp_filter_delete_file that checks the call-stack to determine whether the call originates from wp_delete_attachment or wp_delete_file and return the appropriate filename.

Hope this helps!

[ikappas]

And here is a solution:

	/**
	 * When WordPress removes files, it's expecting to do so on
	 * absolute file paths, as such it breaks when using uris for
	 * file paths (such as s3://...). We have to filter the file_path
	 * to only return the relative section, to play nice with WordPress
	 * handling.
	 *
	 * @param  string $file_path
	 * @return string
	 */
	public function wp_filter_delete_file( $file_path ) {
		$dir = wp_upload_dir();

		if ( $this->func_caller_is( 'wp_delete_file' ) ) {
			return $file_path;
		}

		return str_replace( trailingslashit( $dir['basedir'] ), '', $file_path );
	}

	/**
	 * Determine whether the specified function exists in the call stack.
	 *
	 * @param string $function The function name to check.
	 * @return bool True if the specified function is in the call stack; Otherwise false.
	 */
	protected function func_caller_is( $function ) {
		$stack = debug_backtrace();
		$functions = wp_list_pluck( $stack, 'function' );
		return in_array( $function, array_unique( $functions ) );
	}

[thefrosty]

@ikappas You should open a PR.

[rmccue]

@ikappas This seems like a bug that should be filed with WordPress itself; can you create a ticket on Trac please?

I'd rather we avoid using a backtrace if at all possible. Is there any other way to detect this?

[ikappas]

@rmccue It sure looks like a bug but I wont get into opening a Track ticket. Feel free to do so yourself but take a look at https://core.trac.wordpress.org/ticket/17864 for a similar issue and look at how long it took to fix.

Please don't get me wrong, a Trac ticket should be opened for this. Meanwhile, leaving deleted artifacts behind on S3 is not a good practice.

This issue has been pending here since last April. If you choose to wait for a Track ticket to maybe be resolved via WordPress Core... do the math.

Using backtrace is hacky but gets the job done and I don't know of any other way to go about this.

[mathewemoore]

@ikappas I tested your modification and it worked smashingly on all of my websites using this plugin. Hopefully, this gets fixed in wordpress core, but if not, it's not a huge deal to me. I'm just happy everything gets cleaned up.
http://www.wismusic.com/

[rmccue]

Filed on Trac: https://core.trac.wordpress.org/ticket/39476

Both myself and @joehoyle (the maintainers of this plugin) are guest committers to WordPress core, so we can move relatively fast on this. Ideally, I'd like to try and fix it today to make it into 4.7.1, but if not, it should be in 4.7.2.

[ikappas]

@rmccue Do we have an updated estimate on this issue now that 4.8 is out ?

[jeremyfelt]

I think #215 is a valid enough way forward here without using backtrace until fixed upstream in core.

[joehoyle]

This should have been fixed by #215