Since 4.x release, hook_file_url_alter() is no longer called, breaking several modules

Just calling the hook before processing seems to break things, potentially the query parameters added by Crop API cause issues.

Approach that seems to work is to call the alter on a copy of the URI, and extract the query arguments, and then use them in the returned URLs.

What choice is there if Drupal core just no longer provides this API? 🤔

Drupal core change record from >2 years ago: https://www.drupal.org/node/2940031 → .

This breaks other modules such as Crop API, which add additional query parameters to the URL to ensure that when a crop is changed for an image, the CDN URL is changed accordingly.

Could you explain why this module needs to alter file URLs? 🙏

What choice is there if Drupal core just no longer provides this API?

Sorry I didn’t quite understand the meaning. Do you mean choice for CDN? Or choice for modules like Crop API?

At the moment hook_file_url_alter is not deprecated. Perhaps if we assume it should be then Crop API should perform its own decoration of the generator before CDNs. I raised here though as it looked like a break - as the behaviour changes from what core’s own behaviour is. And the Crop API change would be a separate thing to sort independently.

Could you explain why this module needs to alter file URLs?

If the crop is changed for a file then the styles get flushed so they can be regenerated. For example if the style has a crop resize that results in a different aspect then changing the “crop centre” results in different views of the image within that aspect. The module adds a “h” parameter corresponding to a hash of the crop centre so that any caching proxy properly receives a different “h” for the different derivatives.

At the moment with CDN enabled since 4.x the hash disappears and any change to the crop after initial generation doesn’t reflect on the front end if the CDN is a caching proxy

Driskell: Just calling the hook before processing seems to break things, potentially the query parameters added by Crop API cause issues.

Just tested this, and it's caused by Crop API's hook_file_url_alter converting the URL to FQDN. Taking just the query fixes the issue but doesn't form a nice contract or function in this module. So would need Crop API to not go to FQDN (but it does this to prevent encoding of the query string during conversion to external URL [1]) or would need CDN module to allow for the URL coming back from alteration being external even though the guards in the generate require it to be a stream wrapper.

[1] This is caused by Drupal\Core\StreamWrapper\PublicStream::getExternalUrl() URL encoding the path - so we get a bit deeper now in that in order to allow Crop API to simply add a query string and not convert to external URL - so that CDN can call the alter at the beginning of the function and it still convert to CDN - would need a core change here to separate query string from the path in a public:// scheme URI and not encode it. Not sure why it encodes the path at all though.

We had an edge case where the hook_file_url_alter was needed with urls altered by the cdn module.

We are serving files from s3, but we proxy this through our own apache so end users get these files from our own cdn providers. I was a bit shocked finding out that the cdn module decorates the core FileUrlGenerator and leaves out $this->moduleHandler->alter('file_url', $uri);...

I ended up decorating the cdn FileUrlGenerator with my own FileUrlGenerator, but this pattern could give problems if other modules want to do the same..

OMG I'm so sorry, I was so very wrong in #3! 😱🫣 Clearly I explained too much in a hurry.

I was a bit shocked finding out that the cdn module decorates the core FileUrlGenerator and leaves out $this->moduleHandler->alter('file_url', $uri);...

I am too now! 😰

This is a major, major oversight on my part. But also of https://www.drupal.org/node/2940031 → .

You see, I'm the one who originally introduced hook_file_url_alter() into Drupal core ~15 years ago now: #499156: CDN integration: allow file URLs to be rewritten by hook_file_url_alter() → . Its original purpose was actually … literally to support CDNs. Not to do other file URL transformations.

But it's precisely those other file URL transformations that should still be supported through this hook, the change record I linked is only relevant for the "serve from CDN" use case! 😱 Escalating to critical…

I see the sibling issue in the Crop module has even fewer responses: only you and I, @Driskell, seem to be following it as of today 😅

That explains why so few people have reported this regression: it seems almost nothing in the Drupal ecosystem uses hook_file_url_alter() for anything besides serving from a CDN.

Looking at the code

/**
 * Implements hook_file_url_alter().
 */
function crop_file_url_alter(&$uri) {
  // Process only files that are stored in "styles" directory.
  if (strpos($uri, '/styles/') !== FALSE && preg_match('/\/styles\/(.*?)\/(.*?)\/(.+)/', $uri, $match)) {
    // Match image style, schema, file subdirectory and file name.
    // Get the image style ID.
    $image_style_id = $match[1];
    // Get the file path without query parameter.
    $parsed_uri = UrlHelper::parse($match[3]);
    // Get the file URI using parsed schema and file path.
    $file_uri = $match[2] . '://' . $parsed_uri['path'];

    // Prevent double hashing, if there is a hash argument already, do not add
    // it again.
    if (!empty($parsed_uri['query']['h'])) {
      return;
    }

    if ($crop = Crop::getCropFromImageStyleId($file_uri, $image_style_id)) {
      // Found a crop for this image, append a hash of it to the URL,
      // so that browsers reload the image and CDNs and proxies can be bypassed.
      $shortened_hash = substr(md5(implode($crop->position()) . implode($crop->anchor())), 0, 8);

      // If the URI has a schema and that is not http, https or data, convert
      // the URI to the external URL. Otherwise the appended query argument
      // will be encoded.
      // @see file_create_url()
      $scheme = StreamWrapperManager::getScheme($uri);
      if ($scheme && !in_array($scheme, ['http', 'https', 'data'])) {
        if ($wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($uri)) {
          $uri = $wrapper->getExternalUrl();
        }
      }

      // Append either with a ? or a & if there are existing query arguments.
      if (strpos($uri, '?') === FALSE) {
        $uri .= '?h=' . $shortened_hash;
      }
      else {
        $uri .= '&h=' . $shortened_hash;
      }
    }
  }
}

— https://git.drupalcode.org/project/crop/-/blob/8.x-2.3/crop.module?ref_t...

… I do see one major problem:

      // If the URI has a schema and that is not http, https or data, convert
      // the URI to the external URL. Otherwise the appended query argument
      // will be encoded.
      // @see file_create_url()
      $scheme = StreamWrapperManager::getScheme($uri);
      if ($scheme && !in_array($scheme, ['http', 'https', 'data'])) {
        if ($wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($uri)) {
          $uri = $wrapper->getExternalUrl();
        }
      }

… that would mean that "cropped image" file URLs never actually would have been served from the CDN. But until ✨ Improve far-future support: generate dynamically generated files automatically (f.e. image style derivatives) Needs work is supported, that wouldn't be possible either.

Conclusion: I'm still +1 to restoring this functionality, even though it seems there's only a single contrib module using it.

Tests failed as expected: https://git.drupalcode.org/project/cdn/-/jobs/1494115 👍

Fix just pushed, should be green. Will merge upon green.

@paulvb FYI: this commit will break your decorated file_url_generator service … but in doing so, it should have made yours obsolete.

So you should be able to:

1. delete your code
2. bump the version requirement from drupal/cdn:4.0 to drupal/cdn:4.1

😊

> … I do see one major problem:
> [...]
> … that would mean that "cropped image" file URLs never actually would have been served from the CDN. But until #3179753: Improve far-future support: generate dynamically generated files automatically (f.e. image style derivatives) is supported, that wouldn't be possible either.

Yep, we actually patched this by calling file_url_alter from the CDN module via a patch but only absorbing the query string and then appending it at the end. Of course, not a viable public patch and very specific to our use case of a public CDN that proxy to the site! I think this issue of the URL been turned absolute should be fixed in the crop module now, to ensure it's compatible. There's no reason it should move to external URL that I can think of - or at least if there is a reason it can be dug up in that module.

Thanks for sorting this part of the issue though!

@wim-leers I've investigated a bit and I do actually think we've still got an issue.

public://xxxx stream wrappers cannot contain query strings. The core stream wrapper library will encode the path using UriHelper::encodePath so you cannot have a query string in these URLs.

The way that the core Image module generates URLs with `itok` is that it converts to absolute URI first and then appends it.

With CDN enabled - of course you end up converting to absolute - you have to. But at the same time - it does not support any other module that needs to convert to absolute on the current URI. I'm not sure how to solve this. I don't think there's any way for Crop to support CDN without explicitly having Crop aware that CDN exists.

Ideally, the CDN module would allow absolute URIs at the current BASE URI to be converted to CDN. That way if another module converts to absolute to add query string - like Crop does - during file_url_alter - it would all start working.

If you don't have any objections I'll get around to opening a new CDN issue to note that the FileUrlGenerator should support absolute URIs returned via file_url_alter, converting them back to relative with query string for processing.

@Driskell Could you please open a new issue? 🙏 The functionality was restored to be AFAICT identical to how Drupal core does it … but if there's problems with that approach, we should figure that out in a new issue.

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

Hi @driskell / @wim-leers,

Is there a separate ticket raised for tracking this issue ? we are facing similar issue with CDN recently, In our custom module we are using hook_file_url_alter for processing some of the file/Image urls for one of the sub site but recently we noticed that hook_file_url_alter function is not getting called when we render those images from CDN domain. I upgraded to CDN to 4.1.0 as well but still the issue seems to exist.

Any help would be appreciated.

Thanks,
Harish M.

@wim-leers thanks for the fix, happily using the hook_file_url_alter again!