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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Title: Image styles don't flush on style change with private file download enabled » Image styles don't flush for images stored non-default schemes

There 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']); in image_style_flush

chx’s picture

Status: Active » Needs review
FileSize
983 bytes

Fixing this is not hard.

chx’s picture

That was wrong...

chx’s picture

Version: 7.0 » 8.x-dev
Assigned: Unassigned » chx
Status: Needs review » Needs work
Issue tags: +Needs tests
yareckon’s picture

Status: Needs work » Needs review

#3: image_style_flush_1057656_3.patch queued for re-testing.

yareckon’s picture

Status: Needs review » Needs work

This 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.

yareckon’s picture

Version: 8.x-dev » 7.x-dev

hmm. patch fails to apply on 8.x. Setting version to 7.x where it does apply and where I have tested the fix.

yareckon’s picture

Status: Needs work » Needs review

#3: image_style_flush_1057656_3.patch queued for re-testing.

mondrake’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Can 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

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

This still needs tests, and should be fixed for D8 first.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs backport to D7

#3: image_style_flush_1057656_3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

The last submitted patch, image_style_flush_1057656_3.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
1 KB

new 8.x patch

coredumperror’s picture

I'm using the AmazonS3 module to add the s3:// scheme for storing all my site's files, including image styles. Unfortunately, the use of drupal_realpath() in this function returns an empty string for all "s3://" paths, which prevents file_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 of drupal_realpath() from image_style_flush(), because it absolutely necessitates the use of hook_image_style_flush() just for deleting image styles, which should not be necessary.

fietserwin’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/image.moduleundefined
@@ -889,9 +889,18 @@ function image_style_transform_dimensions($style_name, array &$dimensions) {
+      $schemes[] = $field['settings']['uri_scheme'];

Why 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)

In order for the appropriate error message to be returned this method should not be defined if the wrapper does not support removing directories.

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...

mondrake’s picture

Status: Needs work » Needs review
FileSize
893 bytes

Let's give a try based on input in #15.

Status: Needs review » Needs work

The last submitted patch, image_style_flush_1057656_14.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
948 bytes

New attempt.

fietserwin’s picture

Nice 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.

mondrake’s picture

At the end of the day, why not. Patch attached.

As to the tests, I have not enough experience, so I will pass here.

JacobSanford’s picture

#20: image_style_flush_1057656_20.patch queued for re-testing.

mondrake’s picture

Hi, 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.

mondrake’s picture

Probably better to use image_style_path to build the derivative uri

fietserwin’s picture

Status: Needs review » Needs work

As to the tests, I have not enough experience, so I will pass here.

And 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:

  1. assure we have an image style
  2. create directories for that style (file_prepare_directory()) under several schemes.
  3. call image_style_flush()
  4. check that all directories from 2 are removed

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)?

mondrake’s picture

Status: Needs work » Needs review
FileSize
9.28 KB
5.39 KB

Thanks for review, @fietserwin !

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)?

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.

fietserwin’s picture

Assigned: chx » Unassigned
Status: Needs review » Reviewed & tested by the community

Assuming 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4a1a7f0 and pushed to 8.x. Thanks!

mondrake’s picture

Status: Fixed » Patch (to be ported)

Need backport to D7

mondrake’s picture

Version: 8.x-dev » 7.x-dev

Change version

mondrake’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.18 KB

Patch re-roll for D7

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D7

If 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.

David_Rothstein’s picture

#30: image_style_flush_1057656_30.patch queued for re-testing.

David_Rothstein’s picture

Checking 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:

+  function createSampleImage($style, $wrapper) {
+    static $file;

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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work

The last submitted patch, image_style_flush_1057656_30.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
858 bytes
5.26 KB

Re-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.

coredumperror’s picture

Status: Needs review » Reviewed & tested by the community

I 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).

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/768fe8b

And thanks for the reroll and extra testing also.

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?

Ah, good point - I didn't notice that. Yeah, in that case it could be dealt with in some other issue.

Status: Fixed » Closed (fixed)
Issue tags: -7.23 release notes

Automatically closed -- issue fixed for 2 weeks with no activity.