-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Copy the exported file to a temporary dir instead of the local Media dir #24595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
|
|
App Name | WordPress | |
Configuration | Release-Alpha | |
Build Number | 27873 | |
Version | PR #24595 | |
Bundle ID | org.wordpress.alpha | |
Commit | c52ee05 | |
Installation URL | 4r35f5oaa730g |
|
App Name | Jetpack | |
Configuration | Release-Alpha | |
Build Number | 27873 | |
Version | PR #24595 | |
Bundle ID | com.jetpack.alpha | |
Commit | c52ee05 | |
Installation URL | 2jghek33a4ja0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK, but I think we might be over-complicating this a bit. I suggest capturing the name of the file immediate after it's loaded from the item provider and passing it here:
private func configureMedia(_ media: Media, withExport export: MediaExport) {
media.absoluteLocalURL = export.url
media.filename = export.url.lastPathComponent
This way the URL of the exported/processed file becomes irrelevant.
wdyt?
@kean Do you mean adding this change on top of of this PR, or using your suggested change without this PR? There is another non-user-facing issue, along with the issue of "-1" in the uploaded image. When uploading an image, two image files are copied to the "local media directory": "foo.png" and "foo-1.png", which I'd like to fix too. There is no reason to store unused large files in the app's sandbox. |
I'd probably suggest implementing it as a new/alternative fix, if you agree, of course.
That's a good idea as these could really add up. It needs to remove temporary files after processing and remove everything after upload. There may be some code already doing it. |
Sorry, I was not clear at all in my previous comment. The issue of duplicated image files in the "local media directory" is also fixed in this PR. Considering this PR fixes the issue with the uploaded media file name, and the issue I mentioned above regarding having duplicated image files in the "local media directory", I think we can take this PR's changes for now? |
Description
Fix #23025. This PR supersedes #24570, to address the issue raised in #24570 (comment).
Testing instructions
Verify that issue #23025 is fixed on WP.com sites and self-hosted sites.
Verify that photos, GIFs, and videos can be uploaded.