Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#44563 closed defect (bug) (fixed)

WordPress 4.9.7 Media delete changes break plugins deleting media via stream wrappers

Reported by: antonypuckey's profile antonypuckey Owned by: joemcgill's profile joemcgill
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.7
Component: Media Keywords: has-patch needs-testing needs-unit-tests
Focuses: Cc:

Description

This commit: https://github.com/WordPress/WordPress/commit/c9dce0606b0d7e6f494d4abe7b193ac046a322cd breaks plugins that save media to external systems via stream wrappers.

The underlying cause of this is realpath() does not accept a stream url as valid path and always returns false.

Im not that familiar with wordpress yet but I have created a patch that fixes the problem which i have attached.

Attachments (3)

wordpress-4.9.7-delete-media-streamwrapper.patch (1.5 KB) - added by antonypuckey 6 years ago.
Patch to allow stream wrapper urls to be deleted.
44563.2.diff (1.6 KB) - added by kirasong 6 years ago.
Refresh of patch and spacing.
44563.3.diff (1.6 KB) - added by joemcgill 6 years ago.

Download all attachments as: .zip

Change History (35)

@antonypuckey
6 years ago

Patch to allow stream wrapper urls to be deleted.

#1 @antonypuckey
6 years ago

The side effect of not using this patch is the files that are deleted from wordpress are orhpaned in the remote location and will never be deleted.

#2 @antonypuckey
6 years ago

  • Summary changed from WordPress 4.9.7 Media delete changes break plugins saving media via stream wrappers to WordPress 4.9.7 Media delete changes break plugins deleting media via stream wrappers

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 years ago

#4 @johnbillion
6 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.9.8

Thank you very much for this report, @antonypuckey!

Moving this into the 4.9.8 milestone for investigation.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#6 @pento
6 years ago

  • Milestone changed from 4.9.8 to 4.9.9

4.9.8 is in RC, moving to 4.9.9.

#7 @l3rady
6 years ago

related #44706 and #39476

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 years ago

#9 @joemcgill
6 years ago

@l3rady are you able to test the proposed patch wordpress-4.9.7-delete-media-streamwrapper.patch and confirm that this would fix the issue for you?

#10 follow-up: @pfiled
6 years ago

@joemcgill I'm not l3rady, but I am having this issue with s3-uploads as well. I am out of the office over the holiday weekend here in the US, but will be happy to test it thoroughly on Tuesday.

#11 in reply to: ↑ 10 @joemcgill
6 years ago

Replying to pfiled:

@joemcgill I'm not l3rady, but I am having this issue with s3-uploads as well. I am out of the office over the holiday weekend here in the US, but will be happy to test it thoroughly on Tuesday.

Thanks @pfiled. That would be terrific.

#12 @pfiled
6 years ago

Hi @joemcgill we tested this, and while it removes the original media file, thumbnails are not being removed from S3. Rolling core back to 4.9.6 causes thumbnails to be removed as expected.

I'm in #core-media on slack and happy to test any other changes you suggest.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#14 @pento
6 years ago

#44706 was marked as a duplicate.

#15 @pento
6 years ago

  • Milestone changed from 4.9.9 to 5.0.1

#16 @shadyshane
6 years ago

I encountered the same problem. The reason why S3-Uploads did not work with @antonypuckey 's patch is the WordPress is no longer pass the absolute path to wp_delete_file filter.

S3-Uploads compatible fix for this patch is here https://github.com/nhanledev/S3-Uploads/commit/e19b75629d91113f41cf9ed2fed85ab8b8fb0dd3

#17 @pfiled
6 years ago

@joemcgill Following up on this, I can can confirm that patch @shadyshane provided for s3-uploads combined with this patch does solve this issue.

If it's possible, I'd love to see it in 4.9.9, but I understand if it does not make the cut off.

#18 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#19 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#21 @desrosj
6 years ago

  • Keywords needs-refresh added

#22 @joemcgill
6 years ago

  • Keywords needs-refresh removed
  • Owner set to joemcgill
  • Status changed from new to assigned

#23 @joemcgill
6 years ago

  • Milestone changed from 5.0.3 to 5.1

I've not gotten time to test this one. Going to move to the 5.1 milestone.

@kirasong
6 years ago

Refresh of patch and spacing.

#24 @kirasong
6 years ago

Refreshed patch and added some spacing in 44563.2.diff.

This ticket was mentioned in Slack in #core-media by aaroncampbell. View the logs.


6 years ago

#26 @desrosj
6 years ago

  • Keywords needs-unit-tests added

At a minimum, the tests for `path_is_absolute()` should be updated to account for stream URLs. Additional tests would be great. Streams have been problematic lately.

At a minimum as far as testing is concerned, I'd like to know that the issue is solved in the S3 Uploads plugin with the suggested approach.

@joemcgill I think this is a 5.2 or Future Release candidate, but did not want to punt it without your input.

#27 @joemcgill
6 years ago

  • Milestone changed from 5.1 to Future Release

@desrosj I agree that this will need to slip to a future release. Going to maintain ownership and hope to get this in a future minor.

This ticket was mentioned in Slack in #core-media by jayoswald. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-media by jayoswald. View the logs.


6 years ago

@joemcgill
6 years ago

#30 @joemcgill
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 45177:

Media: Fix deletion of files when using stream wrappers.

This fixes a bug introduced in [43392] which breaks file deletion on systems using stream wrappers, due to limitations of realpath().

Props antonypuckey, pfiled.
Fixes #44563.

#31 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 5.2

#32 @SergeyBiryukov
5 years ago

#46487 was marked as a duplicate.

Note: See TracTickets for help on using tickets.