image_style_url() only prevents page caching

This looks fine for me. But we might should be able to test this.

function image_style_url($style_name, $path) {
  $destination = image_style_path($style_name, $path);

  // If the image already exists use that rather than regenerating it.

  if (file_exists($destination)) {
    return file_create_url($destination);
  }

This code stops to run and never set the cache when $destination does exists.

I think current implementation is right because logic a bit different.

image_style_url() sets cache and URL to 'image/generate/' (causing to call image_style_generate()) and disables page cache only for first time when derivated image will be generated.

So cache entry exists a small amount of time when URL points to image_style_generate() ONLY when derivated image should be regenerated or created.

But when image already exists the image_style_generate() should never be executed!

But that's where the problem begins: you have that URL "inage/generate/..." in your html data and that calls image_style_generate() which returns an access_denied() even if the required image does exist.

@jurgenhaas could you explain a case when image_style_generate() will return access_denied() ? Could you write a test for this case?

As I said above: 'image/generate/...' used ONLY when no image exists and this html is not cached!

So image_style_url() used to return URL and this links could be DIRECT and 'image/generate/...' IN second case this sets cache causing image_style_generate() to generate image so all other calls to image_style_url() will return DIRECT links to file

I think the issue here is you should not be embedding those links in HTML. It looks like re-using the code from theme_image_style() would both ensure the preset is generated if it doesn't exist, and also give you a url which will always work.

http://api.drupal.org/api/function/theme_image_style/7

Downgrading this since I'm sure this works fine otherwise, but we should document that the function should never be used directly to generate HTML output.

I looked at this a bit closer, and I'm putting it back to critical.

image_style_url() prevents stale links from being created via drupal_page_is_cacheable(FALSE), however that's not the only situation where HTML might be cached:

* block caching
* render caching
* panels / views caching
* something like http://drupal.org/project/inline or http://drupal.org/project/filefield_insert where the filter output might be generated with an image/generate path but then cached in the filter module or by text module.

In all of those cases, it'd be possible to cache an image/generate path in the HTML - in fact it's quite likely that you have cache miss for HTML at the same time as a miss for the image style.

I can't think of many ways around this except for adding something like drupal_html_is_cacheable(), which all HTML caching methods could then check before trying to cache_set(), and image.module could use when generating images.

I don't understand image_style_url(). Why do we have different URLs for the case when we need to generate versus the case when we don't. Isn't the whole point to generate an image iff we got a miss? If we can unify on a single URL, then it won't matter if it gets cached.

I believe the reason for having two URLs is so that once a derivative image has been generated we can just link to it directly and let Apache deal with serving the image rather than getting Drupal involved for every image request on the page every single time.

So instead of what we have now at http://api.drupal.org/api/function/image_style_url/7 we would be doing something like (hypothetical):

function image_style_url($style_name, $path) {
  $destination = image_style_path($style_name, $path);

  // If the image already exists use that rather than regenerating it.
  if (file_exists($destination)) {
    return file_create_url($destination);
  }

  // Disable page cache for this request. This prevents anonymous users from
  // needlessly hitting the image generation URL when the image already exists.
  drupal_page_is_cacheable(FALSE);

  // Prepare the $style and $source variables
  [...]

  // Generate the image.
  $destination = image_style_create_derivative($style, $source, $destination);

  return file_create_url($destination);

???

Actually that might not be so good in case you are viewing many images (gallery) and have just flushed a preset, as you would be recreating every single derivative on one page view. Hmm...

MW: Skip this comment. It's just wrong and confusing

I don't think we can keep the current approach for D7. We have lost one of the main advantages of imagecache and thats that derivitave building can no lo lober be off-loaded to a separate web server like media.example.com. With the current code, we force all webservers to generate derivitaves. Once again, the fix is to generate derivitaves during the image request not the original HTML request.

As far as I can see we're still creating derivatives during the image request - the issue is that the url generated for the tag differs depending on whether the file is found or not.

oops. ignore my dumb-ass comment there. thanks catch.

this is still a mess tho.

All of this is taken care of in #827206: image_style_url() only prevents page caching.

This is closed a duplicate of itself?