For multi language private files FileDownloadController is used for the ImageStyle instead of ImageStyleDownloadController

Ok, good news. Found the problem and fixed it. I'll post a patch shortly with the fix and updated tests.

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

IMHO, 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?

The 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?

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

Also, 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) :)

Ah, 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.

Discussed with @dawehner, lets try to add path_processing => FALSE to the URLs generated by PrivateStream?

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

Let's see if testbot like this.

+++ b/core/modules/language/src/LanguageServiceProvider.php
@@ -29,7 +29,7 @@ public function register(ContainerBuilder $container) {
-        ->addTag('path_processor_inbound', array('priority' => 300))
+        ->addTag('path_processor_inbound', array('priority' => 500))

I guess we don't want to alter the path processor priorities now then?

+++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
@@ -144,6 +144,11 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
+    // Make sure that language prefix is never added to the image style URL.q
+    if ($langcode) {
+      $this->assertTrue(strpos($generate_url, "/$langcode/") === FALSE, 'Langcode was not found in the image style URL.');
+    }
+

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

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

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

The 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?

Oops.

I 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:

elseif (strpos($path, '/system/files/styles/') === 0) {
  $path_prefix = '/system/files/styles/';
}
else {
  return $path;
}

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.

Yes, 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.

Attached 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

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

1. Is it healthy for two path processors to have the same priority? (since this is the only issue on this I guess everywhere it works fine, but just for future references)
2. Could it be, that since the private styles do not have a language prefix anymore now, that this might broke existing sites for some reason? Because of some contrib ur custom implementation of some sort?

Regular expression slightly changed to support all langcodes from this list https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Language%...

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

+++ b/core/modules/image/src/PathProcessor/PathProcessorImageStyles.php
@@ -48,8 +48,10 @@ public function processInbound($path, Request $request) {
-    elseif (strpos($path, '/system/files/styles/') === 0) {
-      $path_prefix = '/system/files/styles/';
+    // In case of private file's image style also handle situation when the
+    // language code is prefixed to the path.
+    elseif (preg_match('|^(/[a-z]{2}[\-a-z]*)?/system/files/styles/|', $path, $matches)) {
+      $path_prefix = $matches[0];
     }

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?

How about a very simple solution just by checking the presence of '/system/files/styles/' string inside the $path and altering the $path afterwards?

// Check if the string '/system/files/styles/' exists inside the path,
// that means we have a case of private file's image style.
elseif (strpos($path, '/system/files/styles/') !== FALSE) {
  $path_prefix = '/system/files/styles/';
  $path = substr($path, strpos($path, $path_prefix), strlen($path));
}

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.

I just wanted to reply that #34 resolved the problem for us on AMD.com.

Fixed some coding standards issues. The logic remains.

Hi, I applied the patch of #36 and it solved our problem in Drupal core 8.3.2
Thanks

#36 works well for us too! :)

+++ b/core/modules/image/src/PathProcessor/PathProcessorImageStyles.php
@@ -48,8 +48,10 @@ public function processInbound($path, Request $request) {
+    elseif (preg_match('|^(/[a-z]{2}[\-a-z]*)?/system/files/styles/|', $path, $matches)) {

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

A stupid empty space missmatch prevented applying the patch. so "reroll" by deleting empty space from target section...

Rerolled the patch #41 because it's failed to apply on 8.4.x branch.

This was a nasty one! Thanks, #45 works perfectly!

The feedback from catch in #40 has not yet been addressed.

I forgot about that part as well. Here is the amended patch.

Let's try this again...

@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

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

+++ b/core/modules/image/src/PathProcessor/PathProcessorImageStyles.php
@@ -48,10 +48,11 @@
+    // Check if the string '/system/files/styles/' exists inside the path,
+    // that means we have a case of private file's image style.
+    elseif (strpos($path, '/system/files/styles/') !== FALSE) {
+      $path_prefix = '/system/files/styles/';
+      $path = substr($path, strpos($path, $path_prefix), strlen($path));

You 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?

Not sure how we could simplify that. The thing is in the whole method here:

/**
 * {@inheritdoc}
 */
public function processInbound($path, Request $request) {
  $directory_path = $this->streamWrapperManager->getViaScheme('public')->getDirectoryPath();
  if (strpos($path, '/' . $directory_path . '/styles/') === 0) {
    $path_prefix = '/' . $directory_path . '/styles/';
  }
  // Check if the string '/system/files/styles/' exists inside the path,
  // that means we have a case of private file's image style.
  elseif (strpos($path, '/system/files/styles/') !== FALSE) {
    $path_prefix = '/system/files/styles/';
    $path = substr($path, strpos($path, $path_prefix), strlen($path));
  }
  else {
    return $path;
  }

  // Strip out path prefix.
  $rest = preg_replace('|^' . preg_quote($path_prefix, '|') . '|', '', $path);

  // Get the image style, scheme and path.
  if (substr_count($rest, '/') >= 2) {
    list($image_style, $scheme, $file) = explode('/', $rest, 3);

    // Set the file as query parameter.
    $request->query->set('file', $file);

    return $path_prefix . $image_style . '/' . $scheme;
  }
  else {
    return $path;
  }
}

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.

Makes sense, wasn't visible from the patch context. This was RTBC before and the feedback from @catch has been addressed I think.

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

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!