This ended up being the cause of many problems for me. I discovered the source of my woes being this little "/", despite the fact that there is a strong warning in the field description. I think we just need to fix it in _image_check_settings(). This may fix several issues that seem to only happen for some people. Like this one:
http://drupal.org/project/issues/3256
The attached patch does this.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | image-HEAD.image-path.patch | 3.31 KB | sun |
| #14 | image-HEAD.image-path.patch | 1.8 KB | sun |
| #7 | image_slash_fix2-2.diff | 846 bytes | dalin |
| #5 | image_slash_fix2-1.diff | 789 bytes | dalin |
| #2 | image_slash_fix_2.diff | 945 bytes | dalin |
Comments
Comment #1
drewish commentedi think this is a good change but it'd need to go into 6.x first. at this point i've barely got enough time to just commit patches so i'm affraid i won't be able to work on this.
Comment #2
dalinAgreed. Here's a version of the patch for 6
Comment #3
drewish commentedsorry for the delay but wouldn't
$image_path = rtrim($image_path, '/');do the trick?Comment #4
drewish commentedComment #5
dalinI knew there was something obvious that I was missing. This patch uses the simplified rtrim() method. And at first glance the rtrim further down looks like it would no longer be needed, except if the user leaves the form field blank to use the drupal root (why someone would do that I don't know).
Comment #6
drewish commentedso close ;) $temp_path should be useing the already trimed variable rather than retrimming.
Comment #7
dalinOk, here is a patch with only one rtrim.
Comment #8
jsg commentedI have been struggling for weeks to solve the issue of multiple watchdog messages "Derivative images were regenerated for..." (in Drupal 5) -- and the derivative images not actually being regenerated -- in image. After many hours of fruitless fiddling with variables, upgrades to various modules and hunting through the code, it boiled down to this: one trailing slash too many in the default image path at /admin/settings/image. Who knows how it got there? Probably I put it there and failed to read the small print telling me not to do so.
This patch deserves to be committed to save others the same pain.
Comment #9
drewish commentedcommitted to HEAD. probably needs a re-roll for 5.x-2.x.
Comment #10
sunAww. rtrim() is pretty dirty. We should move that rtrim() into a module update function instead.
Comment #11
dalinAn update solves the problem only once. There are many bugs in the queue describing whacky behaviour possibly caused by having a trailing slash. By having rtirm() here we fix the problem every time a user enters the offending trailing slash into the settings form.
Comment #12
sunThat's what I mean:
variable_get('image_default_path', 'images')should return an already trimmed path in the first place. I.e., the path should be trimmed when the settings form is submitted.All paths in Drupal do not use a trailing slash. So should Image. Fixing it in one place won't prevent us (or any other contrib module) from having to deal with this very unusual case of a path variable.
Comment #13
drewish commentedsun, if you change it, it needs to be done on submit before the variable is saved, just to cut down on support requests. some percentage of the people aren't going to read the instructions and they'll put a trailing / on and it'll break everything. and then there will be 9 duplicate support requests. so better to add a bit of code and trim it down on submit.
Comment #14
sunIf you search for "image_default_path" throughout all files of the module, you will find several instances where the code assumes no trailing slash as well. That's exactly why we need to fix the path before it is stored as variable.
How about this patch?
Comment #15
drewish commentedcan we merge the two submit handlers and rename them? or would that end up hacky?
Comment #16
sunAgreed. Much cleaner now. Committed.