file_url_transform_relative() cannot handle URLs where the port is different from the site's request port

Thanks for the issue and patch. Moving back to needs work for a test.

Added a test, thanks to guidance from dww on #bugsmash.

There is no interdiff because this is so small.

Yay, thanks for working on this. Glad my Slack comments were helpful. ;)

I'm a little reluctant to NW for this review, since it's mostly a reflection of my own confusion. But perhaps we could update the comments to help others make sense of it all?

1. +++ b/core/includes/file.inc
   @@ -126,6 +126,13 @@ function file_url_transform_relative($file_url) {
   +  // Files may be accessible on a unique port.
   

   See below -- I don't think "unique" clarifies the bug here. I was confused until I read the patch and the surrounding code twice.

2. +++ b/core/tests/Drupal/KernelTests/Core/File/UrlTransformRelativeTest.php
   @@ -41,41 +41,56 @@ public function testFileUrlTransformRelative($host, $port, $https, $url, $expect
   -    $data[] = [
   -      'example.com',
   -      8080,
   -      '',
   -      'https://example.com:8080/page',
   -      '/page',
   -    ];
   -    $data[] = [
   -      'example.com',
   -      8443,
   -      'on',
   -      'https://example.com:8443/page',
   -      '/page',
   -    ];
   

   That's weird, looks like we already had test coverage that a file URL with an explicit port is properly handled? Nope, I'm just confused about the bug we're fixing. ;)

3. +++ b/core/tests/Drupal/KernelTests/Core/File/UrlTransformRelativeTest.php
   @@ -41,41 +41,56 @@ public function testFileUrlTransformRelative($host, $port, $https, $url, $expect
   +    $data = [
   +      'http' => [
   ...
   

   +1. It's a bigger diff, but it helps to convert this provider to our usual conventions and to give each case a unique key so it's easier to make sense of failures. I hope no one calls this "out of scope", since I think this is definitely worth keeping.

4. +++ b/core/tests/Drupal/KernelTests/Core/File/UrlTransformRelativeTest.php
   @@ -41,41 +41,56 @@ public function testFileUrlTransformRelative($host, $port, $https, $url, $expect
   +      'http files on unique port' => [
   +        'example.com',
   +        80,
   +        '',
   +        'http://example.com:9000/page',
   +        'http://example.com:9000/page',
   +      ],
   

   I was going to flag this as "weird, why use 80 as the port for the request, but 9000 for the file?", then I realized that's the bug we're fixing! ;)

   The existing test is a little confusing, since file_url_transform_relative() itself is confusing. But the key point here is that these ports should *not* be the same.

   Maybe a comment to that effect, both in these test cases and with the fix? "Unique" didn't exactly clarify this for me, but maybe that's end-of-day brain talking. Something like "non-standard" or "different port than the web request" or something?

Initial stab at a more accurate title for the issue. ;) Further refinements welcome!

@dww, thanks for the review.

Your comment is much better. Added it to file.inc and the array keys for the test. And I noticed that one of the tests should have been using 'https' so changed that.

Excellent, much appreciated, @quietone! Everything now looks really good and clear.

Assuming the bot doesn't contradict me, RTBC. ;)

Thanks,
-Derek

Committed b43c49e and pushed to 9.2.x. Thanks!

Backported to 9.1.x

Whilst the changes to the data-provider are borderline out of scope, I think they improve readability, assist debugging fails (named cases are much simpler to debug) and are a low risk of disruption.

Thanks all