Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In these lines from getExternalUrl(), I am not sure why there is a check for the scheme at all:
// Route public files through Drupal if it looks like they may not exist.
if ($itLooksLikeAFileToMe && $data['scheme'] == 'public') {
//watchdog('hpcloud', 'Giving local path for ' . $data['object']);
return $this->getLastDitchPath($data['object']);
}
// Uh... I guess we give out a URL to the object store?
else {
return $this->container->url() . '/' . $data['object'];
}
If it is right to check the scheme, then I think it is better to check for file_default_scheme() than to hard-code the value 'public'.
I think the standard use-case (and certainly the one I care about) is image styles. Both image_style_path($style_name, $uri) and image_style_flush($style) call file_default_scheme(). (Although image_style_path($style_name, $uri) will use the scheme from the $uri parameter if it is present. I am not sure when this might come up. As I said, I am not sure that the code should be checking the scheme at all.)
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | hpcloud-file_default_scheme-2033367-7.patch | 612 bytes | benjifisher |
| #1 | hpcloud-file_default_scheme-2033367-1.patch | 656 bytes | benjifisher |
Comments
Comment #1
benjifisherThe attached patch replaces
'public'withfile_default_scheme().Comment #2
mbutcher CreditAttribution: mbutcher commentedHave you tested this patch using 'private' as the default scheme?
The problem isn't related to the default scheme, but to the implementation of the 'public' scheme handler. In other words, we're trying to address a problem specific to 'public'. The problem has to do with the way 'public'-scheme URLs are automatically mapped one way, while other schemes are not.
For example, if you change the default scheme to 'private', then (a) the existing code should work as intended, and (b) anything using the scheme 'public' would continue to work as expected.
If, on the other hand, you are finding the same problem with a scheme other than public, let us know. We're not aware of any problems with other schemes.
Comment #3
benjifisherThanks for the quick reply!
I am afraid that I do not understand this remark, even after re-reading the comments on
getExternalUrl().I ran some more tests, with and without the patch that I proposed in #1. In all of my tests so far, I have mapped HPCLOUD to a container using the form at admin/config/services/hpcloud-object, but have not mapped anything else to the cloud. With and without the patch, I tried all three schemes (public, private, hpcloud) for the default download method (admin/config/media/file-system) and all three schemes for the image field (admin/structure/types/manage/article/fields/field_image) for a total of 18 tests.
My test was to edit an article node, removing the existing image file and uploading a new one. If the thumbnail shows up on the edit page, then the test passes. (I also tried saving and viewing the node. As expected, the large image style appears on the node view page exactly when the thumbnail appears on the edit page.)
The proposed patch only makes a difference if both the default scheme and the image field are set to hpcloud. In this case, the test passes with the patch and fails without it.
The other tests all pass if the image field is set to use public or private; they all fail if it is set to use hpcloud.
I ran three more tests, with the more drastic change I suggested (completely removing the test for the scheme). Again, I mapped only HPCLOUD to cloud storage. In these three tests, I set the image field to use hpcloud. These tests all passed, although it took several seconds after the AJAX wheel stopped spinning before the thumbnail image appeared.
Comment #4
mbutcher CreditAttribution: mbutcher commentedDid you have CSS and JS aggregation turned on for these tests?
The issue we ran into is that CSS and JS aggregation makes one set of assumptions about the filesystem while image caching makes a different set. So most of this behavior was born out of attempts to make aggregation and image caching work (with and without CDN) for swift containers.
As I was reading the code, I discovered that some of the Swift bugs that we were working around are now fixed. So your idea to just remove might even be possible now.
Comment #5
benjifisherI did the tests in #3 because you asked
The results are all pretty much what I expect. If I set the image field to use the hpcloud scheme, then
image_style_path()generates a URI with that scheme. (I am 95% sure that it is getting a$uriparameter that includes the scheme.) Then it is pretty clear what the lines quoted in the issue summary do.Comment #6
benjifisherSorry, #5 was a cross post: a follow-up comment on #3, not a reply to #4.
In reply to #4: My previous tests were done with CSS and JS aggregation turned off. (This is a recently created test site, so most things are in the default state.)
I turned on CSS and JS aggregation and did not see any problem. As expected, since I am keeping public as the default scheme and using hpcloud only for my image field. I used the more aggressive patch (removing the test for the scheme).
I will do some more tests soon.
Comment #7
benjifisherFurther testing with the aggressive patch:
Result: my aggregated CSS and JS files are coming from the CDN.
Is that the right test? I am happy to run further tests if it will help.
For the record, I am attaching an actual patch for the aggressive patch.
Comment #8
mfer CreditAttribution: mfer commented@benjifisher Have you configured your setup to have non-public hpcloud containers. For example, private files and/or data outside public and private. An example of that is to have a container that backup and migrate push to.
If you have a container mapped that's not a typical public path that Drupal expects might be local how does that work out?