Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Follow-up of #2658268: Cache persistence after derivative creation
Problem/Motivation
The other issue was got committed with this check:
if (strpos($uri, PublicStream::basePath() . '/styles/') !== FALSE) {
It was discussed quite a bit, so I'm not sure why it was kept in the end.
The problem is that it only works for normal images becaus we call file_create_url() actually twice, the second time in template_preprocess_image(), only then our addition works.
If you use responsive images for example, that second call doesn't happen and responsive images aren't properly updated with the query arguments
Proposed resolution
Not 100% sure yet, possibly move the regex directly into the if.
Remaining tasks
User interface changes
API changes
Data model changes
Comments
Comment #2
BerdirSo, things obviously get a lot more complicated when you try to do it properly.
Looks like our test coverage is still far from complete here.
Notes:
* Moved the regex into the if, with a check about having styles, to avoid running it on anything that is clearly not an image style URL
* I don't understand the code about multiple styles. If there are multiple styles, then the only reason for that would be that they are in a folder called styles, which means the *first* styles is the one to look at, not the last.
* If you do this, then the h argument is added before itok, before it only worked hardcoded by assuming that it was appended to ?itok=...
* Another problem then was that the argument got encoded together with the URL as it was treated as a *path*, so we have to duplicate logic from file_create_url(). This might interfere with other modules that also alter the URL but there's not much we can do about that.
* Also added a check to not do something if there already is a h argument, for example because it gets called twice.
Comment #4
BerdirYeah, that test was strange and was doing things 2 or even 3 times so that the public URL worked, but that also resulted in weird things like the repeated styles path. Cleaned that up.
The test only patch is also the interdiff.
Comment #6
BerdirNot even sure we need both the buildUri() and buildUrl() tests as they are very similar, but doesn't hurt to have them both I think.
Comment #7
d.sibaud CreditAttribution: d.sibaud commentedThe last patch solves my issue, not for responsive image, when I did upload/crop, on save the error shown in the log was:
Source image at public://2017-05/header-servizi-elopement.jpg&h=aefcb2e1 not found while trying to generate derivative image at public://styles/he/public/2017-05/header-servizi-elopement.jpg&h=aefcb2e1.the source image public://2017-05/header-servizi-elopement.jpg was still there, but the url parameter &h=aefcb2e1 was causing the issue.
thanks for the patch.
Comment #8
undertext CreditAttribution: undertext as a volunteer commentedThese regular expressions and URI parsing make me sad.
Is extending Drupal API an option here?
We can just add a small hook into
ImageStyle::buildUrl()\Drupal::moduleHandler()->alter('image_style_url', $url, $this);to allow generated url to be changed based on image style.What do you think about it?
Comment #9
jeroendegloire CreditAttribution: jeroendegloire at Calibrate commentedSame patch but for 2.x branch
Comment #10
woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commentedThanks @jeroendegloire and Big up to @berdir for that patch for obscure reason I haven't seen this issue and any notifications :( Sorry for this bad following...
I have tested all of case and all works as a charm.
@undertext
I prefer the approach of regular expressions in this case, because this hook is called from file_create_url(), and is called fairly frequently (10+ times per page), depending on how many files there are in a given page. We really need to have a performant and fast trick to evaluate if we need to change url created or not.
Comment #13
woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commentedI can confirm everything is ok for both branches and responsive image work well now.
Small screenshot during my test.
@berdir big up :) and thank you all.