If you generate a private file url in another language then the default language the url will contain a langcode. Because of that the wrong controller is used
The request to the file gives a 404. Without the langcode everything is working
Default language relative image url:
/system/files/styles/max_1300x1300/private/images/2016-09/iStock_95128009_XXLARGE.jpg?itok=XZSIigHD
-> this uses ImageStyleDownloadController
Translated node relative image url:
/fr/system/files/styles/max_1300x1300/private/images/2016-09/iStock_95128009_XXLARGE.jpg?itok=XZSIigHD
-> this uses FileDownloadController
Something is wrong with the routing here
Steps to reproduce:
- Install Drupal with standard profile
- Install translation modules
- Add a second language
- make article translatable
- Configure Drupal to use private file storage outside of the docroot
- Configure article node type image field to use private file storage
- Add an article in default language and upload an image
- Translate this article
You can now see the following:
- Thumbnail in translation language node edit form renders perfectly -> that's because it got rendered in the default language first and exists as a file
- If you flush all image styles and go to the translation language node edit form the image style fails to render because Drupal uses the wrong Controller for downloading the imagestyle
Comment | File | Size | Author |
---|---|---|---|
#52 | interdiff-2797639-45-50.txt | 1.03 KB | jzavrl |
#50 | 2797639-50.patch | 8.2 KB | jzavrl |
#48 | 2797639-48.patch | 8.17 KB | jzavrl |
#45 | for_multi_language-2797639-45.patch | 8.17 KB | yogeshmpawar |
#41 | 2797639-41.patch | 8.37 KB | tassilogroeper |
Comments
Comment #2
yobottehg CreditAttribution: yobottehg commentedComment #3
yobottehg CreditAttribution: yobottehg commentedComment #4
yobottehg CreditAttribution: yobottehg commentedComment #5
yobottehg CreditAttribution: yobottehg commentedImageStyleDownloadController is only used for the original Image because of the route /system/files/ .
The image style loads fine for the translated language if the image style url was called before without the language code
Comment #6
jzavrl CreditAttribution: jzavrl commentedComment #7
yobottehg CreditAttribution: yobottehg commentedAdded steps to reproduce
Comment #8
yobottehg CreditAttribution: yobottehg commentedComment #9
jzavrl CreditAttribution: jzavrl commentedOk, good news. Found the problem and fixed it. I'll post a patch shortly with the fix and updated tests.
Comment #10
jzavrl CreditAttribution: jzavrl commentedThe problem was actually in the language module. When matching the request URL with the route, the URL goes through a set of path processors. The image style and the language path processor had the same weight and as such the image style processor ran before the language one. With the patch I have just set a higher weight for the language processor so it runs before the style one.
Comment #11
BerdirIMHO, the problem is that the file has a language prefix in the first place.
That means it can't deliver the actual generated image style file but will always go through the controller/drupal. That's wrong.
Something incorrectly generates the image style URL. It must be done in a way that prevents the language prefix from being added. See \Drupal\image\Entity\ImageStyle::buildUrl(), that doesn't do it, maybe you incorrectly generate those URLs in custom code somewhere?
Comment #12
jzavrl CreditAttribution: jzavrl at WONDROUS commentedThe language prefix on the styles is being added by the core. I've setup a clean install without any other contrib or custom modules and this is the result on both public and private. The language prefix also happens on the original images as well as the files, so I believe that this is the right behaviour.
Do you think language prefixes shouldn't be present on translated images/styles/files? And if so why?
Comment #13
BerdirBecause image styles are generated as physical files so that they only need to go through drupal on the first request which generates them and writes the file. Afterwards, the request must directly deliver the generated file.
It is possible that an automated redirect happens when the file doesn't exist yet if you have redirect with globalredirect features enabled or the corresponding core patch. So this fix does make sense for those cases but it shouldn't happen with just core.
I just check on a production site with two languages and the image URL is correct there.
Comment #14
BerdirAlso, there is no such thing as "translated images/styles/files" (you could have different files per translation, but they're not translated files, the files) :)
Comment #15
BerdirAh, I see. What I said about physical files doesn't actually apply to private files since they have to go through Drupal anyway. Still, it doesn't really make sense for those URL's to have language prefixes but it also doesn't really hurt.
Comment #16
BerdirDiscussed with @dawehner, lets try to add path_processing => FALSE to the URLs generated by PrivateStream?
Comment #17
jzavrl CreditAttribution: jzavrl at WONDROUS commentedSo what are you ideally looking at? To remove the language prefix from the PrivateStream paths?
I've just quickly added path_processing: FALSE to the image.style_private route definition but no luck. I'll try adding it to the URL generation.
Comment #18
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedLet's see if testbot like this.
Comment #19
penyaskitoI guess we don't want to alter the path processor priorities now then?
Comment #20
Berdirany chance that this could result in random fails? I think not since we look for /fr/ any other string that could be random would be much longer or not enclosed like this.
The fix makes sense to me, not sure if we want to change the priority or not.
Comment #21
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedI was thinking about this too. We could try to trim host part from the beginning of it and strictly match beginning of the string. I decided not to go down that route since current solution seemed robust enough.
I guess that changing prio doesn't hurt? Having same prio for two processors sounds like something that could case problems again.
Comment #22
BerdirThe problem with the Priority is that people might have code that specifically directly runs before or after the language path processor and that would then possibly break. Priorities are kind of an API.
If we'd have to change it then fine but if we have a way to avoid this, then maybe better not make a change we don't have to make?
Comment #23
BerdirOops.
Comment #24
jzavrl CreditAttribution: jzavrl at WONDROUS commentedI see your concern regarding the priorities.
So the issue that I found was that the language and the styles path processor had the same priority and because of that the styles one ran first between these two. So that would result an error inside the processInbound() function found in PathProcessorImageStyles.php where you have this chunk:
Here the $path variable would contain something like "fr/system/files/styles/thumbnail/private" and so it wouldn't match it properly. That is why I changed the priority of the processors so the path got cleaned off the language prefix.
Since we now have a patch that removes the language prefix from the image styles we don't really need to change the priority, but I am still a bit concerned about two processors having the same priority and also amazed that this has not caused any other problems in the past.
Comment #25
BerdirYes, making that logic work with/without prefixes might also not be a bad idea.
Would also be good to check compatibility with globalredirect features in redirect module, especially with the new patch that is proposed in #2704213: "Redirect from non-canonical URLs to the canonical URLs" not working with language code in url. I think this route is already problematic and this or a similar one contains a special check in there. That's especially important because that issue is actually a port of a core patch, but we'll see if/how this is failing for that then.
Comment #26
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedAttached patch implements what @berdir proposed in #25. While working on this also found out an issue in default example config for nginx: https://github.com/nginxinc/nginx-wiki/pull/314/files
Comment #27
jzavrl CreditAttribution: jzavrl at WONDROUS commentedAs far as the issue is concerned this looks fine for me now. The styles do not have the language prefix so the image loads, and even if the prefix is there the updated PathProcessorImageStyles also works.
I still have two things on my mind right now.
Comment #28
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedRegular expression slightly changed to support all langcodes from this list https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Language%...
Comment #29
Berdir#27:
1. It's not really healthy, especially if they depend on each other. But I guess we have to live with that now. We could add a @todo for 9.x.
2. I couldn't imagine how. The file is actually generated, so it's not like you can do something like translated image effects that put strings in the image. The first language would win, so you can't rely on a language in that request, IMHO.
I'm wondering if there could be any issue to stop making this match the beginning of the path.
language prefixing is just one use case messing with the path, people sometimes also do regional prefixes, there's projects like https://www.drupal.org/project/purl and so on.
What if we just do a regex on system/files/styles/*/*/, that should be a fairly safe assumption?
Comment #30
jzavrl CreditAttribution: jzavrl at WONDROUS commentedHow about a very simple solution just by checking the presence of '/system/files/styles/' string inside the $path and altering the $path afterwards?
If that string is present, be it with a prefix or suffix, either way, we are looking at a private image style route, and after that we just remove anything before the '/system/files/styles/ so the code below can extract the image style and scheme. Also strpos() is normally faster than preg_match(), and we are doing a very simple check.
Comment #32
yobottehg CreditAttribution: yobottehg commentedThis patch needs a reroll for 8.3.0
Comment #33
yobottehg CreditAttribution: yobottehg commentedlet's look if this still apllies.
Comment #34
yobottehg CreditAttribution: yobottehg commentedHere a reroll with some code style fixes.
Comment #35
AMDandy CreditAttribution: AMDandy commentedI just wanted to reply that #34 resolved the problem for us on AMD.com.
Comment #36
jzavrl CreditAttribution: jzavrl at NDP commentedFixed some coding standards issues. The logic remains.
Comment #37
Isaw CreditAttribution: Isaw at Diputació de Barcelona commentedHi, I applied the patch of #36 and it solved our problem in Drupal core 8.3.2
Thanks
Comment #39
TwoD#36 works well for us too! :)
Comment #40
catchThis seems a bit fragile - it's not a problem unique to language path prefixes. The simple check in #30 that the string is in the path seems a bit more robust.
Comment #41
tassilogroeper CreditAttribution: tassilogroeper at WONDROUS commentedA stupid empty space missmatch prevented applying the patch. so "reroll" by deleting empty space from target section...
Comment #42
tassilogroeper CreditAttribution: tassilogroeper at WONDROUS commentedComment #44
yogeshmpawarComment #45
yogeshmpawarRerolled the patch #41 because it's failed to apply on 8.4.x branch.
Comment #46
seanBThis was a nasty one! Thanks, #45 works perfectly!
Comment #47
idebr CreditAttribution: idebr at ezCompany commentedThe feedback from catch in #40 has not yet been addressed.
Comment #48
jzavrl CreditAttribution: jzavrl at NDP commentedI forgot about that part as well. Here is the amended patch.
Comment #50
jzavrl CreditAttribution: jzavrl at NDP commentedLet's try this again...
Comment #51
idebr CreditAttribution: idebr at ezCompany commented@jzavrl Thanks for creating a new patch. Could you provide an interdiff between the patches for easier reviewing? More information on how to create an interdiff is available at https://www.drupal.org/documentation/git/interdiff
Comment #52
jzavrl CreditAttribution: jzavrl at NDP commentedSure thing, didn't bother as I just added the missing parts from #30, but you're right, either way, it's easier to review. Here is the interdiff.
Comment #53
BerdirYou added a variable now for the substr() but still have the path above in the strpos as well.
Lets either hardcode it for both or use the variable in both places and move it up. The second is probably a bit cleaner/shorter?
Comment #54
jzavrl CreditAttribution: jzavrl at NDP commentedNot sure how we could simplify that. The thing is in the whole method here:
So the $path_prefix is set inside the if statement because it can be different and then used in the preg_replace down the file. The same thing actually happens in the first check. To set it outside would make it redundant in some cases. I think the way it is is probably the most efficient.
Comment #55
BerdirMakes sense, wasn't visible from the patch context. This was RTBC before and the feedback from @catch has been addressed I think.
Comment #57
AnybodyConfirming RTBC. If possible this should be commited to the next D8 release (8.5.0 or .1). Its a big problem for i18n sites. We just ran into it within media. Thank you all for your great work.
Comment #58
catchCommitted/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!