Steps to reproduce:
This patch is an attempt to make this use case work. It can't work if the file system path is not inside Drupal's root directory, because then we don't know if there's a valid URL at all. But if the path is inside Drupal's root directory, we can strip it from the file path, and file uploads also work with public downloads, not only with private ones.
Please review.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | fix-absolute-paths-including-tests.patch | 9.23 KB | jpetso |
| #19 | fix-absolute-paths-including-tests.patch | 7.71 KB | jpetso |
| #14 | simpletest-file-create-url.patch | 10.72 KB | jpetso |
| #13 | fix-absolute-paths.patch | 1.53 KB | jpetso |
| #11 | fix-absolute-paths.patch | 1.56 KB | jpetso |
Comments
Comment #1
jpetso commentedBtw, I tested the patch with all combinations of Public/Private downloads and absolute/relative paths. Everything still works as expected.
Comment #2
keith.smith commentedpatch no longer applies
# patch -p0 < fix-absolute-paths.patch
patching file includes/file.inc
Hunk #1 FAILED at 27.
1 out of 1 hunk FAILED -- saving rejects to file includes/file.inc.rej
Comment #3
c960657 commentedA more general approach is to provide a hook for file_create_url() that allow users to specify their own logic for generating URL's for uploaded files. This will allow all kinds of custom solutions for file storage, including absolute paths to different disk partitions/disk drives, externally hosted files on e.g. Amazon S3 storage, files stored in a database rather than in the filesystem, site-specific file directories for multi-site installations, issues in multi-server setups etc.
Comment #4
jpetso commented@c960657: That would certainly be nice, but I'd consider your proposal a wishlist feature while this issue can be considered a bug. Those two should be separated, imho.
Comment #5
c960657 commentedPerhaps the easiest would be to pass the URLs generated by file_create_url() through custom_url_rewrite() / custom_url_rewrite_outbound().
AFAICS there is currently no way to rewrite the URLs of uploaded files like there is for other files.
Comment #6
c960657 commented@jpetso: Sorry, I didn't see your comment before making my last comment. You are right - the hook stuff belongs in another issue.
Comment #7
jpetso commentedMoving to 7.x... although, I'm currently not so motivated to work on this in the near future.
Comment #8
drewish commentedc960657's approach sounds a lot like #214934: file_url() and hook_file_server()
also wanted to cross link to #238299: file_create_url should return valid URLs which has similar concerns.
Comment #9
drewish commentedhere's a re-roll of jpetso's patch.
Comment #10
keith.smith commentedOne small note on the comment:
I think we normally capitalize URL, and have used "URLs" in lots of other places in core.
Comment #11
jpetso commentedYup, you're right. That comment was the one that you can see at the top of the patch (the reroll does not delete it, while the original patch does) and "urls" has been replaced by "URLs" since I first posted this patch ages ago. Another reroll removes the comment at the top in favor of that in the "private downloads" section, and capitalizes "URLs".
Thanks for the reroll, drewish. Oh, and I haven't yet chimed in with the praise choral for hook_file(), so here's three cheers (or eight) for you! Go Andrew go.
Comment #12
jpetso commentedI'm so glad I wrote those steps to reproduce. Tested again, still works as advertised.
Comment #13
jpetso commentedBetter version, also works with absolute file directory paths + relative file paths, that combination did not work before. And if you're asking yourself how in God's name I discovered *that* rare case, you're right if you're thinking of...
Comment #14
jpetso commentedTest cases! Yay.
Comment #15
jpetso commentedI'm not sure how to handle the static $clean_url variable in url(), though. Commenting out the appropriate test cases might be feasible for personal testing, but probably not for being committed to Drupal core. The test cases should probably go to a separate issue anyways, but hey, it's 2 o'clock in the night, I leave that to someone else.
Comment #16
drewish commentedjpetso see #238299: file_create_url should return valid URLs
Comment #17
jpetso commentedEr, oops. Well at least the tests are sufficiently disjunct so I don't have to be sorry for spending the time to write them.
Comment #18
Anonymous (not verified) commentedThe last submitted patch failed testing.
Comment #19
jpetso commentedThanks to #356721: Regression: Test failures with clean URLs being fixed, we can now enable all the tests. I cleaned up the test class for better read- and maintainability, it's a lot cleaner now. The fix itself still applies and works, as can be witnessed by executing the tests.
Please review and commit! Thanks!
Comment #20
jpetso commentedNote: this issue is pretty different from #238299: file_create_url should return valid URLs, and the tests are too. I therefore labeled mine as "basic tests" (just making sure the URLs come out as expected in all the different public/private vs. clean/unclean URL scenarios), whereas the other issue's tests cover edge cases and actual file storage. Maybe it makes sense to have all of this in a single class, but I'm sure about that, and I think this patch can be perfectly RTBC even without all of that advanced stuff.
Comment #22
jpetso commentedo rly? whyz that?
the tests work at my place... can someone have a look where they fail? please?
Comment #23
jpetso commentedoh, I see, I didn't include the actual fix in the previous patch, just the tests. here's the patch with both tests and fix. please review and commit?
Comment #25
franzLet's update and revive this. If I can read correctly, the bug also existed on 6.x, right?
Comment #26
franz#23: fix-absolute-paths-including-tests.patch queued for re-testing.
Comment #28
franzbump, @jpetso, is this issue still active for D8, this needs to be re-rolled, the function changed a lot.
Comment #29
chemical commentedThe function file_create_url() has been rewritten for D7 and is the same for D8.
The last item on the list in the issue summary might be solve by the patch I have written for #1377840: Caching does not properly respect protocol (a problem when dealing with https).
Comment #36
kim.pepperClosing as outdated. Please re-open if this is still relevant.