References to CSS, JS, and similar files should be relative to the server. Example, in this code:

<link type="text/css" rel="stylesheet" media="all" href="https://drupal.org/files/css/css_1dc04e833f57cb8b13983deeca546394.css" />

Should be:

<link type="text/css" rel="stylesheet" media="all" href="/files/css/css_1dc04e833f57cb8b13983deeca546394.css" />

The attached patch adds a feature to file_create_url() which allows it to create server-relative URLs and puts that feature to work in various core functions.

This change reduces file size, tidies up the code, helps with caching since there does not have to be a different version for HTTP and HTTPS.

CommentFileSizeAuthor
#145 interdiff.txt993 bytesWim Leers
#145 followup_rss_mail-1494670-145.patch7.06 KBWim Leers
#143 followup_rss_mail-1494670-143.patch6.13 KBWim Leers
#140 file_url_transform_relative_enabled-1494670-140.patch1.68 KBBerdir
#137 1494670-revert-137.patch111.02 KBcatch
#109 css-js-relative-urls-1494670-90-8.0.x.patch96.51 KBWim Leers
#90 interdiff.txt1.02 KBWim Leers
#90 css-js-relative-urls-1494670-90.patch96.75 KBWim Leers
#87 interdiff.txt540 bytesWim Leers
#87 css-js-relative-urls-1494670-87.patch96.75 KBWim Leers
#84 interdiff.txt13.61 KBWim Leers
#84 css-js-relative-urls-1494670-84.patch96.24 KBWim Leers
#81 interdiff.txt9.12 KBWim Leers
#81 css-js-relative-urls-1494670-81.patch82.7 KBWim Leers
#81 interdiff-testonly.txt5.06 KBWim Leers
#81 css-js-relative-urls-1494670-81-FAIL-testonly.patch78.67 KBWim Leers
#79 interdiff.txt12.12 KBWim Leers
#79 css-js-relative-urls-1494670-79.patch73.89 KBWim Leers
#77 interdiff.txt14.13 KBWim Leers
#77 css-js-relative-urls-1494670-77.patch62.65 KBWim Leers
#71 interdiff.txt30.11 KBWim Leers
#71 css-js-relative-urls-1494670-71.patch48.05 KBWim Leers
#70 interdiff.txt3.81 KBWim Leers
#70 css-js-relative-urls-1494670-70.patch20.34 KBWim Leers
#68 css-js-relative-urls-1494670-68.patch19.54 KBWim Leers
#62 1494670-62.patch1.38 KBWim Leers
#58 css-js-relative-urls-1494670-58.patch19.59 KBLiam Morland
#56 css-js-relative-urls-1494670-56.patch19.59 KBLiam Morland
#54 css-js-relative-urls-1494670-54.patch19.27 KBLiam Morland
#51 css-js-relative-urls-1494670-51.patch18.77 KBLiam Morland
#47 css-js-relative-urls-1494670-47.patch18.65 KBjhedstrom
#47 interdiff.txt752 bytesjhedstrom
#44 css-js-relative-urls-1494670-44.patch18.61 KBjhedstrom
#44 interdiff.txt666 bytesjhedstrom
#42 css-js-relative-urls-1494670-42.patch18.58 KBjhedstrom
#42 interdiff.txt9.74 KBjhedstrom
#40 css-js-relative-urls-1494670-40.patch9.07 KBjhedstrom
#39 core-css_js_relative_urls-1494670-39-D8.patch9.08 KBLiam Morland
#38 css-js-relative-urls-1494670-38.patch9.08 KBjhedstrom
#38 interdiff.txt1.2 KBjhedstrom
#36 css-js-relative-urls-1494670-36.patch8.44 KBjhedstrom
#36 interdiff.txt919 bytesjhedstrom
#32 css-js-relative-urls-1494670-32.patch8.43 KBjhedstrom
#32 interdiff.txt541 bytesjhedstrom
#31 css-js-relative-urls-1494670-31.patch7.9 KBjhedstrom
#31 interdiff.txt2.96 KBjhedstrom
#28 css-js-relative-urls-1494670-28.patch4.94 KBjhedstrom
#28 interdiff.txt601 bytesjhedstrom
#25 css-js-relative-urls-1494670-25.patch4.36 KBjhedstrom
#25 interdiff.txt1.86 KBjhedstrom
#24 css-js-relative-urls-1494670-24.patch2.49 KBjhedstrom
#24 interdiff.txt995 bytesjhedstrom
#23 css-js-relative-urls-1494670-23.patch1.78 KBjhedstrom
#15 file_create_url_this_server.patch18.43 KBLiam Morland
#13 file_create_url_this_server.patch15.95 KBLiam Morland
#12 file_create_url_this_server.patch14.61 KBLiam Morland
#10 file_create_url_this_server.patch14.57 KBLiam Morland
#8 file_create_url_this_server.patch12.37 KBLiam Morland
#7 file_create_url_this_server.patch623 bytesLiam Morland
#5 file_create_url_this_server.patch11.07 KBLiam Morland
#3 file_create_url_this_server.patch10.61 KBLiam Morland
file_create_url_this_server.patch6.39 KBLiam Morland
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, file_create_url_this_server.patch, failed testing.

Liam Morland’s picture

Sorry, I need to make the tests match the new output.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
10.61 KB

Patch with tests.

Status: Needs review » Needs work

The last submitted patch, file_create_url_this_server.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
11.07 KB

Status: Needs review » Needs work

The last submitted patch, file_create_url_this_server.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
623 bytes

Sorry, I've been having problems with my testing environment. I think I have a working version now.

Liam Morland’s picture

Status: Needs review » Needs work

The last submitted patch, file_create_url_this_server.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
14.57 KB

Trying again.

Status: Needs review » Needs work

The last submitted patch, file_create_url_this_server.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
14.61 KB

My test install is a fresh pull of D8, but I keep getting AJAX errors when I run some of the tests, so I can't do a full test before uploading here. This is one of the error messages I am getting: #1422876: Exception: theme() may not be called until all modules are loaded. in theme().

Liam Morland’s picture

Updated version rolled against latest D8.

Status: Needs review » Needs work

The last submitted patch, file_create_url_this_server.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
18.43 KB
chemical’s picture

Issue tags: +Needs backport to D7

That is one very large patch. I can see what you are trying to do.

The problem is file_create_url() as you have stated but the solution is not to add an argument to the function which will make it backwards compatible though still does not solve the real problem.

The real problem is that the api is buggy which affects other parts of core such as caching #1377840: Caching does not properly respect protocol (a problem when dealing with https). The part of file_create_url(), which prepends the scheme and domain name to the path of files, is a reminiscence from D6 which should not have made it to core into D7 and hopefully never will be a part of D8.

I have supplied a patch for #1377840: Caching does not properly respect protocol (a problem when dealing with https) see comment 18 (some how patch is attached twice) which should do what you want. The solution is not backwards compatible for D7 but does any module relies on the buggy behavior with absolute URLs?

Liam Morland’s picture

Either way is fine with me. I added the parameter because I don't know who else is using the function and needs absolute URLs.

Liam Morland’s picture

Issue summary: View changes

Instead of changing file_create_url(), some of the calls to that function should be wrapped with file_url_transform_relative().

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Definitely needs quite a reroll.

I'm not sure if this is too late for 8.0 or not. The change makes sense to me for all the reasons stated in the summary.

idebr’s picture

The approach in #15 is to add an additional argument to file_create_url() to get a relative url instead of an absolute url.

The approach in #1377840: Caching does not properly respect protocol (a problem when dealing with https) is to make file_create_url() return relative urls for shipped files, which ship as part of Drupal core or contributed modules or themes.

Going with the approach in #1377840: Caching does not properly respect protocol (a problem when dealing with https) would significantly reduce the scope of this issue to internal files with a stream wrapper (eg. public://, private://). The aggregated css/js files are stored in such a stream wrapper.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom

I'll see if I can reroll this and get it working.

jhedstrom’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.78 KB

This will probably have some test failures.

jhedstrom’s picture

FileSize
995 bytes
2.49 KB

Missed one.

jhedstrom’s picture

FileSize
1.86 KB
4.36 KB

Might as well deal with favicon and logos here too.

The last submitted patch, 23: css-js-relative-urls-1494670-23.patch, failed testing.

The last submitted patch, 24: css-js-relative-urls-1494670-24.patch, failed testing.

jhedstrom’s picture

FileSize
601 bytes
4.94 KB

This fixes the failing unit test.

The last submitted patch, 25: css-js-relative-urls-1494670-25.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 28: css-js-relative-urls-1494670-28.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
7.9 KB

This resolves the other failing tests.

I'm not sure this is the best way to go about this, and wonder if it should perhaps be configurable?

jhedstrom’s picture

FileSize
541 bytes
8.43 KB

The previous patch still had absolute file urls in the aggregated CSS files themselves. This iteration addresses that as well.

Liam Morland’s picture

This needs to use $base_path for cases that the Drupal site is not at the root of the server. Or use file_url_transform_relative().

         'form' => 'core/misc/druplicon.png',
-        'src' => $GLOBALS['base_url'] . '/' . 'core/misc/druplicon.png',
+        'src' => '/' . 'core/misc/druplicon.png',
       ),

The last submitted patch, 31: css-js-relative-urls-1494670-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 32: css-js-relative-urls-1494670-32.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
919 bytes
8.44 KB

Good catch! This should address the failing tests.

Status: Needs review » Needs work

The last submitted patch, 36: css-js-relative-urls-1494670-36.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
9.08 KB

This time!

Liam Morland’s picture

jhedstrom’s picture

Status: Needs review » Needs work

The last submitted patch, 40: css-js-relative-urls-1494670-40.patch, failed testing.

jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Status: Needs work » Needs review
FileSize
9.74 KB
18.58 KB

This should fix the tests that were assuming absolute urls to these assets.

Status: Needs review » Needs work

The last submitted patch, 42: css-js-relative-urls-1494670-42.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
666 bytes
18.61 KB

This should fix the failing color test.

The other fail:

fail: [Other] Line 191 of core/modules/system/src/Tests/Common/AttachedAssetsTest.php

passes locally, so I'm not sure what the fix there is.

Status: Needs review » Needs work

The last submitted patch, 44: css-js-relative-urls-1494670-44.patch, failed testing.

The last submitted patch, 44: css-js-relative-urls-1494670-44.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
752 bytes
18.65 KB

These failures are very inconsistent. I was able to reproduce the fails from #44 locally by running in a subdirectory. However, the fail from #42 now appears resolved.

The last submitted patch, 40: css-js-relative-urls-1494670-40.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 47: css-js-relative-urls-1494670-47.patch, failed testing.

The last submitted patch, 42: css-js-relative-urls-1494670-42.patch, failed testing.

Liam Morland’s picture

Liam Morland’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 51: css-js-relative-urls-1494670-51.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
19.27 KB

Fix failing test.

It seems to me that the "There are 2 JavaScript assets in the footer" test should be broken into three tests. This would be a separate issue.

Liam Morland’s picture

Liam Morland’s picture

mijost’s picture

Thank you for the patch.

You can add to the reasons stated in the summary that it fix cross-origin request issues too (domain.com and www.domain.com)

Liam Morland’s picture

Version: 8.0.x-dev » 8.1.x-dev
FileSize
19.59 KB

Reroll.

webchick’s picture

This got brought up in the context of #2606918: [securelogin] Secure Login so calling this a contrib project blocker.

David_Rothstein’s picture

Linking to a related issue which aims to solve a small part of this in a simple way (but I think the part which is most important for contrib modules) - maybe that should get the "Contributed project blocker" tag instead?

mfb’s picture

It's not really a contributed module blocker, as a contrib module like Secure Login can work around it by forcing the URLs to HTTPS. It would be more accurate to tag it "Issue that requires a contributed module to work around".

Wim Leers’s picture

Issue tags: +HTTP/2
FileSize
1.38 KB

Why not just change file_create_url() and PublicStream::getExternalUrl() to return relative file URLs?

(Untested, but intent should be clear.)

Status: Needs review » Needs work

The last submitted patch, 62: 1494670-62.patch, failed testing.

mfb’s picture

These functions have always returned absolute URLs (there are certainly times when you need these - for example, URLs in an e-mail message or RSS feed). So I would say we need a new API for generating relative URLs.

And then whenever the functions that generate absolute URLs are called, we have to make sure we set the url.site cache context.

jhedstrom’s picture

If the approach in #62 works, the tests will still need to be adjusted as in previous patches.

mfb’s picture

As far as work-arounds by contrib modules that I mentioned in #61, it also requires some webserver configuration by the site admin:

If a module forces the URL of a font to use the HTTPS URL, then you get a CORS error when loading a page on the HTTP site:

Font from origin 'https://www.example.org' has been blocked from loading by Cross-Origin Resource Sharing policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://www.example.org' is therefore not allowed access.

So site admins also need to add the Access-Control-Allow-Origin header to various resources on their HTTPS site to allow them to be loaded by the HTTP site... Kind of a PITA :/

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Disregard #62. @mfb is correct in #64.

Part of this is solved by #2375103: Regression: aggregated CSS should use server-relative URLs inside url() calls within the CSS file: that solves the case of file URLs in CSS aggregates.Nope, #60 is right, that is a strict subset of this issue. I really wish I'd started with this issue instead of #2375103, now I've wasted hours reaching the same conclusion: we want to use file_url_transform_relative() for this.

So, +1 for the approach here.

Note that strictly speaking we should also be adding url.path to the required cache contexts. In case the same site is exposed via different base paths. But that's such an extremely, extremely rare case that makes it unnecessary to make that change.

P.S.: the ironic thing is that I am the one (together with Gábor Hojtsy) who introduced file_url_transform_relative() in #2099205: When uploading and inserting an image trough the WYSIWYG plugin a relative path should be used for the image source (src) … Sigh. Our File URL API is really, really bad/weak.

Wim Leers’s picture

Wim Leers’s picture

Just for reference/discoverability, adding the issue that introduced file_url_transform_relative() to the list of related issues.

Wim Leers’s picture

Issue tags: +Needs tests
FileSize
20.34 KB
3.81 KB

Patch review. 99% nitpicks. One actual problem, but it's a test-only problem. What I'm missing, is updated test coverage for CssOptimizer, which I already wrote for the patch in #2375103: Regression: aggregated CSS should use server-relative URLs inside url() calls within the CSS file (which is now marked as a duplicate of this issue). The key problem in HEAD's test coverage for CssOptimizer is that it's relying on absolute paths, even though when Drupal is processing requests, it will never use absolute paths. So the tests aren't reflecting reality.

Furthermore, because this also changes CssCollectionRenderer, CssCollectionRendererUnitTest should also be updated, but isn't, because of the choice of how file_url_transform_relative() got mocked. Currently, that unit test does not accurately test whether file_url_transform_relative() is in fact called or not by CssCollectionRenderer. That should be fixed, and will be in line with the test changes necessary to CssOptimizerUnitTest.

  1. +++ b/core/includes/theme.inc
    @@ -342,11 +342,12 @@ function theme_get_setting($setting_name, $theme = NULL) {
    +
    

    Unnecessary whitespace addition.

  2. +++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
    @@ -168,7 +168,8 @@ public function render(array $css_assets) {
    +
    

    Unnecessary whitespace addition.

  3. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestJavaScriptTest.php
    @@ -43,7 +43,8 @@ protected function doSelectionTest() {
    +      $src = $GLOBALS['base_url'] . '/' . str_replace($GLOBALS['base_path'], '', (string) $script['src']);
    

    This will fail on any Drupal instance that isn't installed in a subdirectory, because the str_replace() will remove every slash, not just the leading slash.

    I also encountered exactly this problem in my patch for #2375103: Regression: aggregated CSS should use server-relative URLs inside url() calls within the CSS file, so in the next reroll I will fix this.

  4. +++ b/core/tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php
    @@ -22,13 +22,16 @@
    -
    ...
    -
    ...
    -
    -
    -
    

    Unnecessary whitespace removal.

  5. +++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
    @@ -32,6 +32,14 @@ function file_uri_scheme($uri) {
    +if (!function_exists('file_url_transform_relative')) {
    +  /**
    +   * Temporary mock of file_url_transform_relative.
    +   */
    +  function file_url_transform_relative($uri) {
    +    return $uri;
    +  }
    +}
    

    Inconsistent whitespace compared to CssCollectionRendererUnitTest.


In this reroll: fixed all nitpicks.

In the next reroll: fixing the bug in UpdatePathTestJavaScriptTest and updating the test coverage for CssOptimizer.

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
48.05 KB
30.11 KB

#70's patch already fixed all points of the review, except for #70.3.

This patch then:

  1. fixes #70.3
  2. fixes CssOptimizerUnitTest (see The key problem in HEAD's test coverage for CssOptimizer is that it's relying on absolute paths, even though when Drupal is processing requests, it will never use absolute paths. So the tests aren't reflecting reality.)
  3. fixes CssCollectionRendererUnitTest (see Currently, that unit test does not accurately test whether file_url_transform_relative() is in fact called or not […] (note this is responsible for the vast majority of the size of this interdiff, and consists mostly of trivial changes)
  4. in fixing the prior point, one bug in #58 was discovered: the case where preprocess == FALSE assets are handled calls file_create_url(), without calling file_url_transform_relative(). So, e.g. CKEditor's loading code would still be affected by this: it'd still have used the absolute URL

As far as I'm concerned, this patch is now RTBC.

Wim Leers’s picture

Component: markup » asset library system
Priority: Normal » Major

Since this stands in the way of some contributed modules and will become increasingly important as HTTP/2 adoption grows, marking as major.

This also does not affect in any way the markup, merely certain attribute values. Moving to the component that seems the best fit.

Wim Leers’s picture

Title: References to CSS, JS, and similar files should be relative to server » References to CSS, JS, and similar files should be relative to server: less bytes, avoids mixed content warnings
Assigned: Wim Leers » Unassigned
Related issues: +#2606918: [securelogin] Secure Login, +#2606032: Port Secure Pages to Drupal 8

Marked the following issues as duplicates of this issue, and made this issue the parent for all of them since they all covered only subsets of what this issue does:

  1. #2375103: Regression: aggregated CSS should use server-relative URLs inside url() calls within the CSS file (major bug, asset library system)
  2. #2611420: Aggregated CSS assumes HTTP, causes mixed content warning when accessed via HTTPS (major bug, asset library system)
  3. #1377840: Caching does not properly respect protocol (a problem when dealing with https) (major bug, cache system)

Combined with the fact that this blocks ports of both the Secure Pages (#2606032: Port Secure Pages to Drupal 8) and Secure Login (#2606918: [securelogin] Secure Login) modules to Drupal 8, I think this is certainly major.

mfb’s picture

Status: Needs review » Needs work

Great. I wasn't aware of file_url_transform_relative() myself - hadn't fully looked at this issue yet. Too bad the resulting code is so verbose.

via grep -r file_create_url core | grep -v Test I found some more calls to file_create_url() that don't bubble the url.site cache context or wrap with file_url_transform_relative().

For example, ImageFormatter.php could be fixed by adding 'contexts' => ['url.site'] to the #cache element or transforming to relative URL.

serg2’s picture

I manually applied the patch in #71 to site on 8.0.2 and it solves the mixed content issues completely.

It seems to me that any site using a SSL termination proxy will encounter mixed content warnings/errors without this patch. Given the popularity of this sort of set-up (think drupal.org & fastly does this) shouldn't this be critical?

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I've been working on addressing @mfb's feedback in #74 since this morning. Several tricky problems. Still, I should be able to finish this today.


I manually applied the patch in #71 to site on 8.0.2 and it solves the mixed content issues completely.

Excellent :)

Given the popularity of this sort of set-up (think drupal.org & fastly does this) shouldn't this be critical?

I wonder the same.

Wim Leers’s picture

Too bad the resulting code is so verbose.

Agreed. We should consider introducing an improved API in 8.1 or 8.2, but retain BC. Of course, that's out of scope for this issue.

I found some more calls

  1. In this reroll, I also wrapped the file_create_url() calls in the following places in a file_url_transform_relative() call:
    1. \Drupal\Core\Render\Element\ImageButton::preRenderButton()
    2. TwigExtension
    3. \Drupal\ckeditor\Plugin\Editor\CKEditor::getJSSettings()
    4. \Drupal\ckeditor\Plugin\Editor\CKEditor::buildContentsCssJSSetting()
    5. color_block_view_pre_render()
    6. \Drupal\file\Plugin\Field\FieldFormatter\UrlPlainFormatter::viewElements()
    7. \Drupal\file\Plugin\views\field\File::renderLink()
    8. _responsive_image_image_style_url()
    9. \Drupal\responsive_image\Plugin\Field\FieldFormatter\ResponsiveImageFormatter::viewElements()
  2. Just a comment was necessary in one case: \Drupal\file\Plugin\Field\FieldFormatter\RSSEnclosureFormatter::viewElements().
  3. Completely unnecessary file_create_url() calls and in fact even unwanted ones were removed in two cases, because they operate on a value that was returned by file_create_url() already: responsive_image_build_source_attributes()
  4. In one case, I added the url.site cache context because there is a strong expectation of absolute URLs: file_tokens().
  5. In three cases, I added the url.site cache context because because #type => link/LinkGenerator/Link requires a
    Url object to be passed, and Url objects do not support relative URLs: they require fully qualified URLs. Those three cases are:
    1. template_preprocess_file_link()
    2. \Drupal\file\Plugin\Field\FieldFormatter\BaseFieldFileFormatterBase::viewElements()
    3. \Drupal\file\Plugin\Field\FieldFormatter\FileUriFormatter::viewValue()
  6. Finally, in one case, I should add the url.site cache context, but I can't: \Drupal\file\Plugin\views\field\File::renderLink().

To fix the underlying problem, I opened #2646744: \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links. Fixing that is out of scope here. The places in this patch that have temporary work-arounds that will be better fixable as part of #2646744 have @todos indicating so.

This fixes 99% of the problem.

Status: Needs review » Needs work

The last submitted patch, 77: css-js-relative-urls-1494670-77.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
73.89 KB
12.12 KB

Test failures, yay! They show we have decent test coverage for this. Now updated the test expectations, this should now be green & RTBC'able again :)

mfb’s picture

Status: Needs review » Needs work

we're getting close :)
file_create_url() is used in a few more places for images:
core/includes/theme.inc: template_preprocess_image()
core/modules/image/image.admin.inc: template_preprocess_image_style_preview()
core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php: ImageFormatter::viewElements()
core/modules/image/src/Entity/ImageStyle.php: ImageStyle::buildUri()

Wim Leers’s picture

Thanks!

  • Updated template_preprocess_image() and its test coverage. Note that here, the existing test coverage was once again incorrect. (It used /core/misc/druplicon.png instead of core/misc/druplicon.png, thus already not reflecting reality.)
  • Updated template_preprocess_image_style_preview() (I skipped it in #77 because it is used for an admin form and is never cached, but you're right that it should also be fixed)
  • UpdatedImageFormatter::viewElements(): great catch! I can't believe I missed that one.
  • ImageStyle::buildUri() doesn't have any file_create_url(), I think you meant ImageStyle::buildUrl(). This one I did not forget about, I explicitly didn't change it. Because:
    1. Its documentation explicitly says that it returns an absolute URL.
    2. Consequently, changing it now would be a BC break.
    3. Finally, it's the result of that function that needs to be passed through file_url_transform_relative(). And that's in facht what #77 already did. For example:
         $entity = ImageStyle::load($style_name);
         if ($entity instanceof Drupal\image\Entity\ImageStyle) {
      -    return $entity->buildUrl($path);
      +    return file_url_transform_relative($entity->buildUrl($path));
         }
      

    But, what would be valuable/useful there, is for ImageStyle::buildUrl() to reference file_url_transform_relative() just like file_create_url() does. So I did that.

Status: Needs review » Needs work

The last submitted patch, 81: css-js-relative-urls-1494670-81.patch, failed testing.

The last submitted patch, 81: css-js-relative-urls-1494670-81-FAIL-testonly.patch, failed testing.

Wim Leers’s picture

Clearly there were even more test cases that I didn't find yet (because I didn't run the entire test suite locally). This updates the other tests' expectations.

Wim Leers’s picture

Title: References to CSS, JS, and similar files should be relative to server: less bytes, avoids mixed content warnings » References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send

Better title.

mfb’s picture

yes, buildUrl(). One other thing to flag: File::url() in core/modules/file/src/Entity/File.php calls file_create_url() - I'm not sure if it's used anywhere or what could be done about it.

Wim Leers’s picture

\Drupal\Core\Entity\EntityInterface::url() says Gets the public URL for this entity.. But file URLs as they are stored/handled/passed in Drupal use stream wrappers (public://, private://), which only make sense privately (internally), not publicly. So, it totally makes sense that it's converted from public://cat.jpg to http://example.com/sites/default/files/cat.jpg thanks to file_create_url(). And it then depends on the context where it is used whether it makes sense to use a root-relative URL or not.

For further convincing/confirmation: it also has \Drupal\file\FileInterface::getFileUri(), which returns the URI, i.e. public://cat.jpg.

In other words: the same situation as for \Drupal\image\ImageStyleInterface::buildUrl(). What we did there, was add an @see file_url_transform_relative(), so I think it makes sense to do the same here.

mfb’s picture

Title: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send » References to CSS, JS, and similOn

Read thru it quickly and the only problem I noticed is a typo in a @todo: couldadd

mfb’s picture

Title: References to CSS, JS, and similOn » References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send

oops :/

Wim Leers’s picture

Fixed.

P.S.: if all that's holding back an RTBC is a few typos, you can just RTBC it :) Either a committer can fix it, or the developer could reroll it with just those typos fixed.

mfb’s picture

Status: Needs review » Reviewed & tested by the community

Tested and this resolves mixed-content warnings and CORS errors on a site with multiple base URLs, so setting to RTBC.

Things are quite broken if you access a site with multiple base paths - e.g. http://localhost/d8/ and http://d8.local/ - but that's probably out of scope of what Drupal supports.

catch’s picture

Issue tags: +Needs change record

So just to confirm, if a site is accessed with two different base paths, '/' and '/foo' then it'll be broke, whereas currently it isn't.

I don't really mind breaking that in a minor release, can't think of any possible use-case (except for something like a dev environment with shared db and filesystem, but separate code bases on local machines, which is a stretch), and this is a legitimate bug report that affects lots of actually existing sites (and is a small improvement for every site).

However, that feels like something we could solve if we made the base path part of the CSS hash keys maybe? If it's not that simple, not really worried about it.

We should have a change record though - both to recommend using file_url_transform_relative() and noting the behaviour change.

mfb’s picture

To clarify, core is already broken for multiple base paths without this patch.

I was just mentioning that this patch doesn't improve things for that edge case.

Two problems I've seen:

  • One of the sites will try to redirect to the other site and you get an error: "Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it."
  • Menu links result in 404s, for example you get a link like http://d8.local/d8/user/logout for a site available at http://d8.local/ and http://localhost/d8/
catch’s picture

Oh that makes sense and is fair enough.

One more question:

+++ b/core/modules/system/src/Tests/Theme/ImageTest.php
@@ -32,9 +33,17 @@ class ImageTest extends KernelTestBase {
+    // The code under test uses file_url_transform_relative(), which relies on
+    // the Request containing the correct hostname. KernelTestBase doesn't set
+    // it, so push another request onto the stack to ensure it's correct.
+    $request = Request::create('/', 'GET', [], [], [], $_SERVER);
+    $this->container = $this->kernel->getContainer();
+    $this->container->get('request_stack')->push($request);
+

Shouldn't we always set this in KernelTestBase? Seems like a reasonable expectation that it would be set.

Wim Leers’s picture

Shouldn't we always set this in KernelTestBase? Seems like a reasonable expectation that it would be set.

Yes, and I was initially changing KernelTestBase. But, this could theoretically break some tests. So I didn't. Happy to move it though.

catch’s picture

That kind of change is OK in a minor I think, so as long as it doesn't actually break tests I'd do it here. But also fine if we open explicit follow-up to do it.

Wim Leers’s picture

Then I'd prefer to do it in a follow-up. It's a problem that's out of scope here, and probably merits its own discussion to consider possible consequences.

catch’s picture

Status: Reviewed & tested by the community » Needs work

OK that's fair. Just needs the change record and follow-up created then. Marking needs work for that.

Wim Leers’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

  • catch committed 4a320ea on 8.1.x
    Issue #1494670 by Liam Morland, jhedstrom, Wim Leers, mfb: References to...
Wim Leers’s picture

Hurray!

But what about 8.0? This affects sites currently being built also, and fixes it for them, see #75 for example.

Markup remains the same, only attribute values change — and are arguably a bug fix.

Liam Morland’s picture

Thanks very much, everyone!

Is this eligible for backport to D7?

tim.plunkett’s picture

In our Page Manager test coverage, we use the location of the logo.svg to determine which theme is being used.
I can get creative and find a way that works for both, I just wanted to point out the inconvenience of having this differ between versions.

#2651060: Result of PageManagerAdminTest::assertTheme() differs between 8.0.x and 8.1.x

mfb’s picture

The CORS errors and mixed-content warnings are severe enough to warrant backporting to 8.0 imho

the one silver lining of not backporting could be encouraging more sites to make the switch to HTTPS only.. :)

catch’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Fixed » Needs review

Re-opening for 8.0.x for now, this is very likely non-breaking but want to give it another look for patch-release things before committing anything.

catch’s picture

Status: Needs review » Reviewed & tested by the community

So #104 is the sort of breakage that if it affected production code would make sense to keep this 8.1.x only.

But since that's only test breakage because we fixed a bug, and there's no other change here that ought to be breaking, I'm putting it back to RTBC for 8.0.x. Will give it a day in case something else comes up.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 90: css-js-relative-urls-1494670-90.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
96.51 KB

#90 rebased against 8.0.x, with thanks to git's 3-way merging — I had to manually fix only one hunk.

  • catch committed 37d4477 on 8.0.x
    Issue #1494670 by Liam Morland, jhedstrom, Wim Leers, mfb: References to...
catch’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.0.x, thanks!

Wim Leers’s picture

Published the CR at https://www.drupal.org/node/2650374 now that this has also been committed to Drupal 8.0.x. It will hence be available to all Drupal 8 sites starting from version 8.0.3. Yay for mixed content warnings being a thing of the past!

cilefen’s picture

Nice job everyone on this issue.

benjy’s picture

+++ b/core/modules/responsive_image/responsive_image.module
@@ -495,9 +495,9 @@ function _responsive_image_image_style_url($style_name, $path) {
+  return file_url_transform_relative(file_create_url($path));

It's sad for DX IMO that you have to remember to call both these functions to not run into issues. It would have been better to have just one method that you can call with $path that returns the relative URL.

Wim Leers’s picture

@benjy: I agree. But it's important to get this fixed ASAP. This required zero API changes or additions.

We should open a follow-up issue to improve the DX of all that, but doing so will require at least an API addition, and preferably a new service to make it all much more sensible. With these two functions to be continued to be supported, to not break BC.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Liam Morland’s picture

@#114 My original patch added a $this_server parameter to file_create_url(). #16 and #20 asked that it be done with file_url_transform_relative().

benjy’s picture

@Liam, yeah that was my first thought as well although the docs on file_create_url are quite explicit about it being a remote URL.

As Wim says, we should definitely create a follow-up to discuss DX improvements.

fmeirelesfon’s picture

The recent changes to TwigExtension.php are not consistent with the file_url documentation, which states that "This helper function accepts a relative path from the root and creates an absolute path to the file."

Also, down the road this causes issues with the link function, "InvalidArgumentException: The URI '/portal/sites/default/files/2016-01/file.pdf' is invalid. You must use a valid URI scheme. in Drupal\Core\Url::fromUri()"

TR’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Patch (to be ported) » Active

Well wait a minute. Now there's no way to generate an image tag with an absolute URL to a local image. This is a regression, and it broke my code.

My use case is I'm sending e-mail with images. The images are local, but the URLs MUST be absolute so that they show up in the recipient's MUA.

    $logo = array(
      '#theme' => 'image',
      '#uri' => Url::fromUri('base:' . $uri, ['absolute' => TRUE])->toString(),
    );

My Url::fromUri() call returns the correct absolute URL to my image, but because of the call to file_url_transform_relative() in template_preprocess_image(), #uri will ALWAYS contain a relative path and will NEVER have the full, absolute URL.

So the only way to do this now is to hand-code an image tag? How about adding something like '#absolute' in there so I can get an absolute URL when I want one?

Wim Leers’s picture

#121:
You can already do exactly what you want:

Before (your current code that is now broken):
 $logo = array(
      '#theme' => 'image',
      '#uri' => 'public://image.jpg',
    );
After
 $logo = array(
      '#theme' => 'image',
      '#uri' => file_create_url('public://image.jpg'),
    );

The docs even say this:

 *   - uri: Either the path of the image file (relative to base_path()) or a
 *     full URL.
catch’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Active » Patch (to be ported)
TR’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Patch (to be ported) » Active

You can already do exactly what you want

No I can't, which is why I'm posting. It used to work properly before this patch was committed, now it doesn't. This is both a regression and a bug, because this patch changed the behavior and because it now doesn't behave as documented. Please read what I wrote more carefully.

I use 'example.org' as my site name. If you test this you have to change this to *your* site name - it's an important distinction because the wrong behavior only shows up when you are using URLs with your own site name.

On 21 Jan my code generated e-mail with this content, which correctly displayed the image in the e-mail:
<img src="http://example.org/core/themes/seven/logo.svg" alt="" typeof="foaf:Image" />

On 10 Feb the exact same code generated this (wrong) content instead:
<img src="/core/themes/seven/logo.svg" alt="" typeof="foaf:Image" />

After bisecting, I traced the problem to this issue.

You quoted documentation at me, but I already know what the documentation says. If you read my post you will see I DID pass a fully-qualified URL, so it SHOULD have worked according to the documentation. And it DID work until this change.

Let me be more explicit:

    $logo = array(
      '#theme' => 'image',
      '#uri' => 'http://example.org/core/themes/seven/logo.svg'
    );

If you run this code on 'example.org' it generates:
<img src="/core/themes/seven/logo.svg" alt="" typeof="foaf:Image" />
This is wrong. (Note the FULL URL as required by the documentation).

In #121 I pointed out that the #url argument is passed through file_url_transform_relative(), and it's this IMPLEMENTATION that doesn't conform to the documentation for template_preprocess_image() that you quoted. Read the documentation for file_url_transform_relative(), it says it's going to transform the URL to a relative URL, and it does.

benjy’s picture

This also broke Entity Print which used to use the absolute URLs to get at assets, fixed in the latest version for D8 but just wanted to point it out.

mfb’s picture

Arguably many URLs should be absolute URLs by default, and vary the cache by site (aka base URL), because there are so many contexts where absolute URL is needed (e-mail, embed code, feeds, content export, etc.). There is some downside to caching per base URL, but that's something a site with multiple base URLs just has to deal with.

I don't think anyone has made a followup issue to improve DX yet, but I'd say for anything that generates a URL, developers should be able to pass a flag indicating whether a relative or absolute URL should be generated. It's confusing that this is doable in some cases but not others.

Wim Leers’s picture

Assigned: Unassigned » catch

If you read my post you will see I DID pass a fully-qualified URL, so it SHOULD have worked according to the documentation. And it DID work until this change.

You're right. My bad. Sorry, I read over that.


The root cause of the problems here is that Drupal generates file URLs without knowledge about the context they will be used in. Because 99% of the time, relative file URLs are preferable. It's only for edge cases like sending e-mails, RSS and printing that you want absolute URLs.

Assigning to catch to get his thoughts.

catch’s picture

Arguably many URLs should be absolute URLs by default, and vary the cache by site (aka base URL), because there are so many contexts where absolute URL is needed (e-mail, embed code, feeds, content export, etc.). There is some downside to caching per base URL, but that's something a site with multiple base URLs just has to deal with.

That seems like a reasonable option to fix the regression while avoiding mixed-mode warnings etc.

Wim Leers’s picture

  1. #128: Given the existing CR at https://www.drupal.org/node/2650374, that'd mean reverting most of the patch committed here and instead updating core's services.yml to include the url.site cache context. That'd be a simple solution, but…
  2. I'd much rather fix the problem at the root, by making sure file_create_url() knows what context it is being used in, or by allowing the developer to explicitly state they want absolute URLs (like @TR asked in #121). This is also what we do for UrlGenerator (for generating non-file URLs). For non-file URLs, #2335661: Outbound path & route processors must specify cacheability metadata ensured the url.site cache context is bubbled when explicitly generating absolute URLs.
    As part of doing that, we could introduce FileUrlGenerator, a sibling for UrlGenerator. file_create_url() would then be the procedural wrapper/BC layer for generating file URLs.
    Created an issue for this: #2669040: Ability to generate absolute file URLs.
  3. Finally, I'd like to call out that actually, it seems quite fair to always use root-relative file URLs inside HTML. Because Drupal-generated HTML is to be sent over HTTP 99.9% of the time, 0.01% of the time, it is actually sent in an e-mail or as part of RSS. It'd be trivial for those few edge cases to parse URLs and make them absolute. i.e. just reimplement the behavior that browsers already perform while interpreting relative URLs.
    They will need to do this anyway if they're sending out user-generated content, which could already contain relative URLs (file URLs or content URLs)!
Berdir’s picture

It's certainly not ideal, but might be a lot easier to implement than other solutions:

What if we add some kind of state to file_url_transform_relative() that you can control and basically use to disable that function. For example in MailManager::mail(), we'd say that all links from here until at the end of the function have to be absolute, so we disable the transforming. Views could to the same in the rss display plugin and so on.

Wim Leers’s picture

Already started #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it, which we probably should do regardless of which of the four options listed in #129/#130 we choose.

Liam Morland’s picture

I'd much rather fix the problem at the root, by making sure file_create_url() knows what context it is being used in, or by allowing the developer to explicitly state they want absolute URLs (like @TR asked in #121).

My original patches for this issue added a boolean parameter to file_create_url() which did this (actually, it was the other way around, with the default being absolute so as to not change the default behavior).

Berdir’s picture

Yes, but callers might not know the context themself either. E.g when using the contact module to send mails or simplenews, an image/file formatter won't know that it is being rendered for an email.

Which is why I suggested a separate static flag that can be set by the code who *does* know that an email is now sent, or an RSS feed printed.

cilefen’s picture

@Liam Morland I posted a patch on the other issue - can you see if it makes any sense? I didn't actually test it...

catch’s picture

#1 seems like the least risky option for 8.0.x given the regression - although we need to ensure we don't break things for modules that already updated. So I'd be tempted to try that then continue on #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it for 8.1.x/8.2.x

TR’s picture

Priority: Major » Critical

We no longer have a way to generate an image tag with an absolute URL to a local image.

If this regression isn't going to be fixed, this commit should be reverted until a better solution can be found.

I don't see any evidence that the people who broke this are interested in fixing the mistake. I'm all for progress in Drupal core, but if you unintentionally break functionality that has existed in core for at least 10 years, you ought to take some responsibility and make the fix your priority.

catch’s picture

@TR personally attacking people working on core issues does not help with motivation. This wasn't a new feature that got added, it was a major bug fix that had unintended consequences. If every bug introduced into core was only worked on by the people who introduced the bug in the first place, we'd still be on Drupal 3 or earlier. This isn't the only issue where I've seen you re-open issues due to regressions with personal attacks rather than any attempt to propose a fix.

Retrospectively, it would have been much better to commit this to 8.1.x only and see how it went there (assuming no-one spotted the regression before commit still) before cherry-picking to 8.0.x, and it's unfortunate this was only noticed after a release had been tagged, but too late for that now.

This no longer reverted cleanly. So starting with a revert patch. If this is green, we should add the url.site cache context to core.services.yml per #129-1.

Status: Needs review » Needs work

The last submitted patch, 137: 1494670-revert-137.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
Berdir’s picture

But the url.site context won't be enough.

We also need to re-open the css/js issues that were closed as duplicate to vary those files by something similar to url.site. And there might be other things too.

Maybe we could revert this only in 8.0.x and instead try a way forward in 8.1.x+? But we might be too close to the release of that, so maybe 8.2.x+. We'll probably have to accept that mixed mode will not properly work with aggregated CSS/JS in 8.0.x anyway.

here's a quick proof of concept for my idea. Would of course be a lot nicer as part of the new #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it.

Edit: For the css/js problem, what if we only do a partial revert and keep the calls for those, as we can probably safely assume that they will only be called (and only work for) normal http responses?

The last submitted patch, 137: 1494670-revert-137.patch, failed testing.

mfb’s picture

I'd agree that CSS files should contain relative URLs in the aggregated CSS, as they do in Drupal 7.

Wim Leers’s picture

@catch suggested more than a month ago in comment #135 to revert this patch for 8.0.x and apply the url.site cache context to the entire site instead. But then most modules and sites will already have updated at this point. So it's probably safer to just add a absolute === TRUE||FALSE variable to the image template/preprocess function, as @TR suggested in #121.

That could be acceptable for 8.0, but not for 8.1/8.2/8.x, as I explained in:

  1. #127: the root cause of this mess (this unanticipated regression): the fact that Drupal generates file URLs without knowledge about the context those URLs will be used in. But as @Berdir pointed out in #133, the place where a file URL is generated in may itself not know the context either! For example, an image may be rendered into HTML and then that HTML may happen to be sent in an e-mail, and therefore the URL should be absolute. But that was impossible to know at the time when the image template was being rendered (for example when rendering an article to be sent via e-mail, the rendering of the article is not at all different: it's still rendered to HTML).
  2. #129: any time you're sending HTML in a non-web context (for example: e-mail, RSS), you have to make sure all URLs are absolute. But file_create_url() is only invoked on files referenced by code. A complete solution would already have to make all relative URLs absolute anyway, including those in user-generated content. Note that this has been a bug in Drupal since 2006: #88183: Relative URLs in feeds should be converted to absolute ones.

@Berdir's solution in #140 also is inadequate: the problem with that approach is that it breaks render caching. It's global state, for which you'd need to define a cache context, and the problem is that this cache context could change at any time while processing a request. So even that doesn't work.


Given all of the above, AFAICT the only sensible thing we can do, is solve the problem at its root. Write a response event subscriber (a response filter in Symfony terminology) that updates all relative URLs in RSS responses to be absolute. For e-mail, we'll need a different approach, because it's not treated as a response. Perhaps that's the solution: do invoke a KernelEvents::RESPONSE event with the appropriate MIME type, so we can implement a response event subscriber for that also.

Attached patch implements the RSS portion. Verified that it passes feed-related tests, had to update only two bad test assertions.

Status: Needs review » Needs work

The last submitted patch, 143: followup_rss_mail-1494670-143.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.06 KB
993 bytes

One more such bad assertion.

Berdir’s picture

I agree that my approach is flawed. But it does work right now for images in emails, while the last patch doesn't cover mails yet, just RSS if I understand correctly.

I'll test available patches asap for our use case, for now, we'll use my workaround.

benjy’s picture

I had something similar in entity print that was using HTML5() because \DOMDocument() doesn't work for HTML5.

Can we make the solution generic so that I can use it in contrib as well? and maybe that will come as part of supporting mail as Berdir suggests.

Wim Leers’s picture

Can we make the solution generic so that I can use it in contrib as well? and maybe that will come as part of supporting mail as Berdir suggests.

+1. But the 99% use cases are covered by letting core do this for RSS + e-mail out of the box.

I'd love @Berdir's thoughts on how to apply the #145 pattern to MailManager::mail(), I find it very difficult to understand how that logic is supposed to work. It's quite clear it has its roots in Drupal 6 or even 5.

Berdir’s picture

We also just had this on a site that we updated to the latest Drupal 8 version that returns generated HTML to be displayed in an app that was broken by this change.

That wouldn't be fixed either without adding custom code.

I'm not sure either how to apply Wim's patch to mails either. It's not a response and while it's always fun to blame Drupal 5/6, it will conceptually always be something that's rendered inline and then sent by a pluggable backend. We could try to transform it between the alter and actually sending it out, but that could break safe markup strings and cause possibly other problems too, not sure (theoretically, someone could write a custom backend that supports things like only rendering render arrays there)

Wim Leers’s picture

Ok, so then I will have to spend the time necessary to figure out how to make this work for e-mails.

I stand by my analysis in #143:

any time you're sending HTML in a non-web context (for example: e-mail, RSS), you have to make sure all URLs are absolute. But file_create_url() is only invoked on files referenced by code. A complete solution would already have to make all relative URLs absolute anyway, including those in user-generated content. Note that this has been a bug in Drupal since 2006: #88183: Relative URLs in feeds should be converted to absolute ones.

Reverting the committed patch is not a solution, fixing that generic problem for the two non-web contexts we have (e-mail + RSS) is what we should do.

Assuming I find a way to make this work for e-mail, is everybody +1?

Wim Leers’s picture

Shall I move the patch in #145 to #88183: Relative URLs in feeds should be converted to absolute ones, so we can get that committed already? And then open a new equivalent issue for e-mails?

Wim Leers’s picture

13:13:19 <catch> WimLeers: +1 to #151
13:13:33 <catch> WimLeers: then we should close that issue as fixed.

Done.

  1. #88183-53: Relative URLs in feeds should be converted to absolute ones
  2. #2704597: Relative URLs in mails should be converted to absolute ones
Fabianx’s picture

Okay, so to resolve the critical part of the regression the following should be done by a wizard with mad preg_replace / sed skills:

- Add a new helper function:

file_create_relative_url()

Add to the docs:

"This function will create a relative url via file_create_url when not explicitly an absolute url is passed in."

Use this helper function in all cases where file_url_transform_relative(file_create_url()) was used.

Add the following code:

function file_create_relative_url($uri) {
  $scheme = file_uri_scheme($uri);

  // Do not transform absolute URLs to relative ones.
  if ($scheme == 'http' || $scheme == 'https') {
    return file_create_url($uri);
  }

  return file_url_transform_relative(file_create_url($uri));
}

Because I agree with TR, if you put in an absolute URL explicitly, Drupal should not transform it to a relative one and the two function calls loose information:

- $url = http://www.myhost.com/great-article // file_create_url($url) == $url, file_transform_relative_url(file_create_url($url)) == /great-article
- $url = /great-article // file_create_url($url) == http://www.myhost.com/great-article, file_transform_relative_url(file_create_url($url)) == /great-article

benjy’s picture

+1 as well but i'd still like to see some code that can be re-used between other modules that need to do this if possible.

Fabianx’s picture

I created #2705405: [Regression] Developers are unable to specify absolute URLs within #image, twig, ... as follow-up for #153.

This is completely independent of other URLs needing to be absolute but being relative at this moment.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Version: 8.0.x-dev » 7.x-dev
Priority: Major » Normal
Status: Closed (fixed) » Patch (to be ported)

This was marked for backport several times above. Moving the backport to a separate followup issue that links to this one may be desirable rather than continuing here.

My recollection is that the mixed-content warnings were mostly a D8-only problem in practice, but this still might qualify as a bugfix for Drupal 7 due to #1377840: Caching does not properly respect protocol (a problem when dealing with https) and #1032210: drupal_render_cid_parts() should add http vs. https protocol to cache keys (issues that were marked duplicate of this one, though I'm not entirely sure they should remain closed as duplicates either).

Wim Leers’s picture

Moving the backport to a separate followup issue that links to this one may be desirable rather than continuing here.

Yes, please.

andypost’s picture

Version: 7.x-dev » 8.0.x-dev
Assigned: catch » Unassigned
Priority: Normal » Major
Status: Patch (to be ported) » Closed (fixed)
David_Rothstein’s picture

Issue tags: -Needs backport to D7

Removing backport tag - thanks for creating the followup issue.