To reproduce:
1. upload some images
2. set up an image style
3. set up images to display with the style
4. edit the image style, note that images change to new style when viewed
5. switch to private file downloads
6. edit image style again, note that images *don't* change to new style, previously cached versions are used.
Expected behaviour can be seen with public downloads set.
1. @admin/config/media/file-system set downloads to public
2. upload some images
3. create an image style, display images with image style
4. change image style
5. on next image viewing the updated style is used
Workaround:
1. edit image style
2. switch back to public downloads
3. view images to cache updated versions
4. switch back to private downloads
I'm using php 5.3 on ubuntu 10.04, imagemagick.module and toolkit set to imagemagick. Though flipping back to GD as the toolkit appeared to make no difference.
Marking this as a bug, but is this just Drupal's same old "you can't switch to private downloads once you've chosen public or you'll have issues"? I thought Drupal-7 was supposed to be beyond that?
Comment | File | Size | Author |
---|---|---|---|
#36 | image_style_flush_1057656_36.patch | 5.26 KB | mondrake |
#36 | interdiff_30-36.txt | 858 bytes | mondrake |
#30 | image_style_flush_1057656_30.patch | 5.18 KB | mondrake |
#25 | image_style_flush_1057656_25.patch | 5.39 KB | mondrake |
#25 | interdiff_23-25.txt | 9.28 KB | mondrake |
Comments
Comment #1
chx CreditAttribution: chx commentedThere is more to this: it is not about switching back and forth: if you have an image field set to anything but the default scheme then the flush will not work due to default scheme hardwired:
$style_directory = drupal_realpath(file_default_scheme() . '://styles/' . $style['name']);
inimage_style_flush
Comment #2
chx CreditAttribution: chx commentedFixing this is not hard.
Comment #3
chx CreditAttribution: chx commentedThat was wrong...
Comment #4
chx CreditAttribution: chx commentedComment #5
yareckon CreditAttribution: yareckon commented#3: image_style_flush_1057656_3.patch queued for re-testing.
Comment #6
yareckon CreditAttribution: yareckon commentedThis seems likst a straightforward fix. chx, I am queueing this for retesting, and hopefully it can be just committed for 7.x and 8.x.
Comment #7
yareckon CreditAttribution: yareckon commentedhmm. patch fails to apply on 8.x. Setting version to 7.x where it does apply and where I have tested the fix.
Comment #8
yareckon CreditAttribution: yareckon commented#3: image_style_flush_1057656_3.patch queued for re-testing.
Comment #9
mondrakeCan you please commit this? I tested it and IMHO it is RTBC.
The fact that images stored in private schema do not flush is spreading, see
http://api.drupal.org/api/drupal/modules%21image%21image.module/function...
http://drupal.org/project/imagestyleflush
Thanks
Comment #10
tim.plunkettThis still needs tests, and should be fixed for D8 first.
Comment #11
mondrake#3: image_style_flush_1057656_3.patch queued for re-testing.
Comment #13
mondrakenew 8.x patch
Comment #14
coredumperror CreditAttribution: coredumperror commentedI'm using the AmazonS3 module to add the
s3://
scheme for storing all my site's files, including image styles. Unfortunately, the use ofdrupal_realpath()
in this function returns an empty string for all "s3://" paths, which preventsfile_unmanaged_delete_recursive($style_directory)
from ever running.Looking at the
drupal_realpath()
docs reveals that it's supposed to be deprecated, and is intended to be removed from all Drupal core functions, because it can't handle stream wrappers for remote file systems. I propose that this patch should incorporate said removal ofdrupal_realpath()
fromimage_style_flush()
, because it absolutely necessitates the use ofhook_image_style_flush()
just for deleting image styles, which should not be necessary.Comment #15
fietserwinWhy not add the scheme as key as well and then remove the array_unique?
Another idea is to just go through all stream wrappers that are registered in Drupal (not php). That list is probably shorter than the list of fields and covers the case where a style (temporarily) not in use but does have image derivatives of the given style. This can lead to outdated image files when the style becomes referenced by a field (again).
Regarding the drupal_realpath being deprecated:
- is_dir() does a stat() internally and can thus be called with stream wrapper notation (stream_stat() is part of the PhpStreamWrapperIntrerface).
- file_unmanaged_delete_recursive() seems to be able to handle stream wrapper notation.
So drupal_realpath() can just be removed?
However, not all stream wrappers need to implement rmdir: (http://php.net/manual/en/streamwrapper.rmdir.php)
Not sure what (appropriate) error message is returned if s3:\\ (or any other (remote) stream wrapper) doesn't support rmdir. Let's hope it is not a fatal...
Comment #16
mondrakeLet's give a try based on input in #15.
Comment #18
mondrakeNew attempt.
Comment #19
fietserwinNice clean up. Another minor improvement might be to remove is_dir(), as file_unmanaged_delete_recursive() also does an is_dir(). The difference might be that if it is a file (absolutely not expected and not wanted here), it is removed allowing to create it as a directory the next time a image derivative is created.
Regarding the tests:
- We should test a local writable stream (public?)
- And a remote writable stream wrapper?
- My earlier remark about stream wrapper not implementing rmdir can be ignored. Assuming that each stream wrapper defined in Drupal implements PhpStreamWrapperInterface, it MUST implement rmdir, albeit by throwing an exception (which is not according to the PHP recommendations, it should trigger E_WARNING's). But it might still be interesting to test a stream wrapper that does not really implement it.
Comment #20
mondrakeAt the end of the day, why not. Patch attached.
As to the tests, I have not enough experience, so I will pass here.
Comment #21
JacobSanford#20: image_style_flush_1057656_20.patch queued for re-testing.
Comment #22
mondrakeHi, a re-roll, now with some test coverage. The test creates derivative images in both 'public' and 'private' stream wrappers and then changes the image style, which results in the style being flushed. After flush, we expect to find no images in any wrapper, with the exception of the 'sample' image used for previewing in the style form.
Comment #23
mondrakeProbably better to use image_style_path to build the derivative uri
Comment #24
fietserwinAnd now you tried anyway. Very good!
I have also not that much experience with writing tests, but IMHO tests should only test the feature at hand, in this case: image_style_flush(). So a simpler test should:
IMHO, this is all there is to testing image_style_flush(). We are e.g. not testing file_unmanaged_delete_recursive(), so it is not necessary to first add some files to that directory. You could add tests for checking that the proper caches are cleared, but that would need a more elaborate specification of what image_style_flush is expected to do (and when it gets called).
Regarding the current patch:
- no newline at end of file
- should the test be renamed from ImageFlushStyleTest to ImageStyleFlushTest (following the name of the function)?
Comment #25
mondrakeThanks for review, @fietserwin !
Fixed here.
Re. you other points, I feel we are getting in a discussion around functional tests vs. unit tests. This is beyond me, so I'll pass again :)
The current test proves the issue I was having on D7 - i.e. if I generate derivative of images stored in private (e.g. because I have an image field that specifies 'Private files' as upload destination), and then change the image style, the 'old' derivatives are still displayed.
Comment #26
fietserwinAssuming functional tests, even then, IMHO, we are testing far too much at once, because the tests use DrupalPost (thus submitting page requests) instead of making calls to the appropriate image API functions. As such, the sample image derivative gets recreated before we can test that the directory is actually completely flushed.
But, if others don't mind, this is RTBC because the bug is solved and there is a test. If others do mind, they can set it back to needs work.
Comment #27
alexpottCommitted 4a1a7f0 and pushed to 8.x. Thanks!
Comment #28
mondrakeNeed backport to D7
Comment #29
mondrakeChange version
Comment #30
mondrakePatch re-roll for D7
Comment #31
fietserwinIf it is OK for D8, it also is so for D7.
I compared the D7 and D8 patch and basically they are the same, except that in D8 an image style is an entity, and thus $style->id() is used and a style has a separate label, whereas D7 needs to use $style['name'] for identifying and presenting a style.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commented#30: image_style_flush_1057656_30.patch queued for re-testing.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedChecking if this still passes tests after #606598: Human readable image-style names (I decided to commit that one first, since it's a much bigger patch and the issue has also been around longer... sorry.)
Even if tests manage to pass, we probably should reroll this one to use the new image style labels in the test anyway.
Otherwise, this looks great to me (nice tests), but has anyone looked into the issue discussed in #14 and #15 regarding what happens on remote stream wrappers? Has anyone manually tested this patch at all? :)
I also found this use of "static" a little strange:
Shouldn't it be drupal_static(), or nothing at all? I guess it might not matter much in practice (unless you are managing to run the same test twice in the same session) but doesn't seem like a static variable really belongs in a function like that at all. Could probably be pushed to a followup issue.
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedComment #36
mondrakeRe-roll to catch up with style label.
Re. usage of static, I was proudly copying from similar function in ImageAdminStylesUnitTest. If it has to change, it should probably change there as well, and in D8 first. Follow-up?
Re. manual testing and remote wrappers - fair points. I checked myself with public and private but have no way to check remote ones. Anyway that should be confirmed by a reviewer I guess.
Comment #37
coredumperror CreditAttribution: coredumperror commentedI can confirm that this patch fixes the problem for users of the AmazonS3 module's s3:// scheme. I've posted a patch to that module's issue queue which alters its implementation of
hook_image_style_flush()
, since it no longer needs to delete the remote files itself (when used with this patch).Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/768fe8b
And thanks for the reroll and extra testing also.
Ah, good point - I didn't notice that. Yeah, in that case it could be dealt with in some other issue.