Incompatibility with CDN 4.x

Created on 4 October 2023, 9 months ago
Updated 11 June 2024, 5 days ago

Problem/Motivation

Since CDN module 4.x, which changes file URLs to be from a different domain (which can be a caching proxy), and has options for forever caching files too, the Crop "h" parameter is no longer part of the CDN file URL.

This is because hook_file_url_alter is no longer called by CDN module as it replaces core FileUrlGenerator for files.

Steps to reproduce

1. Install CDN module and configure it to enable it
2. Setup Crop API, for example using the Focal Point module
3. Load a page
4. No "h" parameter on any images

Proposed resolution

Posting this issue here and linking to raised one in CDN about the missing hook.
https://www.drupal.org/project/cdn/issues/3391254 πŸ› Since 4.x release, hook_file_url_alter is no longer called, breaking several modules Active

But another resolution is for this module to not use hook_file_url_alter and instead, similar to CDN module, decorate the FileUrlGenerator and add the hash. It would need to ensure that this module runs after CDN though so perhaps not a good solution. Just needs some decision on how to play nicely together

Remaining tasks

-

User interface changes

None

API changes

Possible change to way "h" is added.

Data model changes

None

πŸ› Bug report
Status

Postponed: needs info

Version

2.0

Component

Code

Created by

πŸ‡¬πŸ‡§United Kingdom Driskell

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @Driskell
  • Status changed to Needs review about 1 month ago
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    πŸ› Since 4.x release, hook_file_url_alter is no longer called, breaking several modules Active was fixed and will ship in the 4.1 release. Can you please test again and confirm that the problem is gone? 😊

  • Status changed to Postponed: needs info about 1 month ago
  • πŸ‡¬πŸ‡§United Kingdom Driskell

    @wim-leers Thanks! CDN now calls the URL alter - so that's one step forwards.

    However that's the first part and as you noted in #3391254 the images won't be converted to CDN urls:

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

    I think though this can be now easily worked on here. I can't see a specific reason why the crop module should change the URL to an external one. I can see in the comments in the code it's relating to the query string getting converted somewhere - so could be a workaround for some other issue that needs fixing first too.

          // 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()
    

    I'll investigate this to see if perhaps it is an old issue that was already fixed, or if it needs a fix, and report back when I can. I'm not the maintainer but postponed until that info is available is way forwards.

  • πŸ‡¬πŸ‡§United Kingdom Driskell

    I think this still requires CDN change - as Crop has to add a query string - but core stream wrapper getExternalUrl actually encodes the path including query string so the query becomes part of the final path part public://styles/file.png%3Dquery which then breaks the final path. The image style module builds its URL by converting to absolute first then adding `itok`. It feels like a big core change to support query on stream wrapper paths. So I don't think Crop is doing anything wrong here.

    I think CDN needs to check after the file_url_alter in canServe for the current base URI and support that as a valid path to convert to CDN. Otherwise I can't see this working well without Crop being "CDN-aware" which not sure it should be.

  • πŸ‡ΊπŸ‡ΈUnited States Harish.04

    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.

  • πŸ‡¬πŸ‡§United Kingdom Driskell

    @Harish.04

    Latest version does see file_url_alter get called. It calls at the point that the CDN URL is generated rather than when the image is pulled from the CDN.

    I did raise this issue on CDN now though but I think it's not your issue as the hook does run, it's just CDN refuses to handle an absolute URL: https://www.drupal.org/project/cdn/issues/3453869 ✨ Support absolute URLs that point to public stream wrapper files in FileUrlGenerator Active

  • πŸ‡ΊπŸ‡ΈUnited States Harish.04

    Hi @driskell,

    yeah they have mentioned hook_file_url_alter is restored in CDN 4.1.0 version but doesn't seem to be fixing our issue. In our case we are altering the file path in hook_file_url_alter in our custom module (basically stripping our sub domain prefix from base url) but it doesn't seem to be working.
    It calls at the point that the CDN URL is generated rather than when the image is pulled from the CDN. - so the custom code written for hook_file_url_alter doesn't get called when image is pulled from the CDN.

  • πŸ‡¬πŸ‡§United Kingdom Driskell

    @Harish.04 I'm not sure I can help very much but the hook won't receive the absolute URL unless some other hook has expanded it. For an image it would receive public://2024-05/file.jpg as an example. So you can't really strip a prefix. If you're looking to alter what CDN is doing then I think you need to use a different method as CDN is not using hook_file_url_alter for a long time and instead overrides the FileUrlGenerator. Maybe previous in much older versions it did and in that case you may have seen the CDN url pass through the hook - but that is not the case anymore by design.

  • πŸ‡ΊπŸ‡ΈUnited States Harish.04

    Thanks @driskell I will check on different options for overriding the File URL's.

Production build 0.69.0 2024