![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Changes between Initial Version and Version 5 of Ticket #60788
- Timestamp:
- 08/29/2025 12:21:16 PM (14 months ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
Ticket #60788
- Property Keywords has-patch 2nd-opinion needs-testing needs-unit-tests added
-
Ticket #60788 – Description
initial v5 1 1 2 In https://core.trac.wordpress.org/changeset/51939 a change was made to resolve ticket https://core.trac.wordpress.org/ticket/38231 to make `download_url()` use the `Content-Disposition` header to specify the name of the downloaded file. I realize I'm a bit late to the party here since the ticket was opened more than 7 years ago and the change was made more than 2 years ago, but I think that this change was flawed and needs to be reconsidered (and possibly reverted entirely). The way it was implemented seems fundamentally broken and has the effect of making `download_url()` basically impossible to use reliably.2 In [51939] a change was made to resolve ticket #38231 to make `download_url()` use the `Content-Disposition` header to specify the name of the downloaded file. I realize I'm a bit late to the party here since the ticket was opened more than 7 years ago and the change was made more than 2 years ago, but I think that this change was flawed and needs to be reconsidered (and possibly reverted entirely). The way it was implemented seems fundamentally broken and has the effect of making `download_url()` basically impossible to use reliably. 3 3 4 4 Consider the following simple PHP script named `echo.php`: … … 67 67 }}} 68 68 69 You might argue that the code for `download-bad.php` is flawed, and should be rewritten to look more like `download.php`, and I would agree. However, note that `download-bad.php` would have worked fine before the change https://core.trac.wordpress.org/changeset/51939was made. Furthermore, even `download.php` is likely to display buggy behavior if two users attempt to run it at the same time. I don't think there is any reliable way to use `download_url()` in a manner that will protect against concurrent access (short of adding some kind of external locking mechanism). Consider the following PHP files, `foo.php`...69 You might argue that the code for `download-bad.php` is flawed, and should be rewritten to look more like `download.php`, and I would agree. However, note that `download-bad.php` would have worked fine before the change [51939] was made. Furthermore, even `download.php` is likely to display buggy behavior if two users attempt to run it at the same time. I don't think there is any reliable way to use `download_url()` in a manner that will protect against concurrent access (short of adding some kind of external locking mechanism). Consider the following PHP files, `foo.php`... 70 70 71 71 {{{#!php … … 105 105 I'm not really sure what should be done with `download_url()`, but as it currently stands it seems essentially impossible to use in a reliable manner. Personally I would probably recommend just removing all `Content-Disposition` support entirely. That would technically be a backward compatibility break, but I'm not sure anyone is actually using this functionality? To me it doesn't really even make any sense as currently implemented. Why would you want a temporary file stored under a fixed name? 106 106 107 It looks like the `Content-Disposition` feature was originally proposed as a solution to the problem that `download_url()` uses `wp_tempnam()` in such a way that it can create very long filenames for some URLs, as described in https://core.trac.wordpress.org/ticket/34938 - but that doesn't make much sense either:107 It looks like the `Content-Disposition` feature was originally proposed as a solution to the problem that `download_url()` uses `wp_tempnam()` in such a way that it can create very long filenames for some URLs, as described in #34938 - but that doesn't make much sense either: 108 108 109 1. The change to use the `Content-Disposition` header bears no resemblance to the change that was originally requested in https://core.trac.wordpress.org/ticket/34938109 1. The change to use the `Content-Disposition` header bears no resemblance to the change that was originally requested in #34938 110 110 2. Adding support for the `Content-Disposition` header doesn't actually fix the original problem (what if the URL being downloaded doesn't have a `Content-Disposition` header?) 111 111 3. Even if the URL being downloaded has a `Content-Disposition` header, the way the `Content-Disposition` support was implmented means that it still creates a file based on the URL first, then renames it later, so the first file could still have a filename which is too long 112 4. Another change was made in https://core.trac.wordpress.org/changeset/37598which should (mostly) address the original problem112 4. Another change was made in [37598] which should (mostly) address the original problem 113 113 114 114 That having been said, the original patch https://core.trac.wordpress.org/attachment/ticket/38231/38231.diff actually makes a lot more sense than what was finally committed. The original patch takes the `Content-Disposition` filename and feeds it as input into `wp_tempnam()`, so you will get a random filename which should prevent the issues in the examples I've given. Somehow, in the multiple iterations of the patch, the call to `wp_tempnam()` got lost, so the current code just uses the `Content-Disposition` filename directly instead of using a random filename based on the `Content-Disposition` filename.
