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.
Comment | File | Size | Author |
---|---|---|---|
#145 | followup_rss_mail-1494670-145.patch | 7.06 KB | Wim Leers |
#109 | css-js-relative-urls-1494670-90-8.0.x.patch | 96.51 KB | Wim Leers |
Comments
Comment #2
Liam MorlandSorry, I need to make the tests match the new output.
Comment #3
Liam MorlandPatch with tests.
Comment #5
Liam MorlandComment #7
Liam MorlandSorry, I've been having problems with my testing environment. I think I have a working version now.
Comment #8
Liam MorlandComment #10
Liam MorlandTrying again.
Comment #12
Liam MorlandMy 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().
Comment #13
Liam MorlandUpdated version rolled against latest D8.
Comment #15
Liam MorlandComment #16
chemical CreditAttribution: chemical commentedThat 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?
Comment #17
Liam MorlandEither way is fine with me. I added the parameter because I don't know who else is using the function and needs absolute URLs.
Comment #18
Liam MorlandInstead of changing file_create_url(), some of the calls to that function should be wrapped with file_url_transform_relative().
Comment #19
jhedstromDefinitely 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.
Comment #20
idebr CreditAttribution: idebr commentedThe 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.
Comment #21
idebr CreditAttribution: idebr commentedComment #22
jhedstromI'll see if I can reroll this and get it working.
Comment #23
jhedstromThis will probably have some test failures.
Comment #24
jhedstromMissed one.
Comment #25
jhedstromMight as well deal with favicon and logos here too.
Comment #28
jhedstromThis fixes the failing unit test.
Comment #31
jhedstromThis 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?
Comment #32
jhedstromThe previous patch still had absolute file urls in the aggregated CSS files themselves. This iteration addresses that as well.
Comment #33
Liam MorlandThis 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().
Comment #36
jhedstromGood catch! This should address the failing tests.
Comment #38
jhedstromThis time!
Comment #39
Liam MorlandReroll.
Comment #40
jhedstromRe-rolling.
Comment #42
jhedstromThis should fix the tests that were assuming absolute urls to these assets.
Comment #44
jhedstromThis 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.
Comment #47
jhedstromThese 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.
Comment #51
Liam MorlandReroll.
Comment #52
Liam MorlandComment #54
Liam MorlandFix 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.
Comment #55
Liam MorlandTest is divided-up in #2601790: In AttachedAssetsTest.php, separate a 3-part test into 3 separate test assertions.
Comment #56
Liam MorlandReroll.
Comment #57
mijost CreditAttribution: mijost commentedThank 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)
Comment #58
Liam MorlandReroll.
Comment #59
webchickThis got brought up in the context of #2606918: [securelogin] Secure Login so calling this a contrib project blocker.
Comment #60
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedLinking 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?
Comment #61
mfbIt'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".
Comment #62
Wim LeersWhy not just change
file_create_url()
andPublicStream::getExternalUrl()
to return relative file URLs?(Untested, but intent should be clear.)
Comment #64
mfbThese 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.
Comment #65
jhedstromIf the approach in #62 works, the tests will still need to be adjusted as in previous patches.
Comment #66
mfbAs 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:
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 :/Comment #67
Wim LeersDisregard #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 usefile_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.Comment #68
Wim LeersFirst, a straight reroll. This is just #58 rebased.
Comment #69
Wim LeersJust for reference/discoverability, adding the issue that introduced
file_url_transform_relative()
to the list of related issues.Comment #70
Wim LeersPatch 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 forCssOptimizer
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 howfile_url_transform_relative()
got mocked. Currently, that unit test does not accurately test whetherfile_url_transform_relative()
is in fact called or not byCssCollectionRenderer
. That should be fixed, and will be in line with the test changes necessary toCssOptimizerUnitTest
.Unnecessary whitespace addition.
Unnecessary whitespace addition.
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.
Unnecessary whitespace removal.
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 forCssOptimizer
.Comment #71
Wim Leers#70's patch already fixed all points of the review, except for #70.3.
This patch then:
CssOptimizerUnitTest
(see )CssCollectionRendererUnitTest
(see (note this is responsible for the vast majority of the size of this interdiff, and consists mostly of trivial changes)preprocess == FALSE
assets are handled callsfile_create_url()
, without callingfile_url_transform_relative()
. So, e.g. CKEditor's loading code would still be affected by this: it'd still have used the absolute URLAs far as I'm concerned, this patch is now RTBC.
Comment #72
Wim LeersSince 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.
Comment #73
Wim LeersMarked 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:
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.
Comment #74
mfbGreat. 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.Comment #75
serg2 CreditAttribution: serg2 commentedI 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?
Comment #76
Wim LeersI'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.
Excellent :)
I wonder the same.
Comment #77
Wim LeersAgreed. 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.
file_create_url()
calls in the following places in afile_url_transform_relative()
call:\Drupal\Core\Render\Element\ImageButton::preRenderButton()
TwigExtension
\Drupal\ckeditor\Plugin\Editor\CKEditor::getJSSettings()
\Drupal\ckeditor\Plugin\Editor\CKEditor::buildContentsCssJSSetting()
color_block_view_pre_render()
\Drupal\file\Plugin\Field\FieldFormatter\UrlPlainFormatter::viewElements()
\Drupal\file\Plugin\views\field\File::renderLink()
_responsive_image_image_style_url()
\Drupal\responsive_image\Plugin\Field\FieldFormatter\ResponsiveImageFormatter::viewElements()
\Drupal\file\Plugin\Field\FieldFormatter\RSSEnclosureFormatter::viewElements()
.file_create_url()
calls and in fact even unwanted ones were removed in two cases, because they operate on a value that was returned byfile_create_url()
already:responsive_image_build_source_attributes()
url.site
cache context because there is a strong expectation of absolute URLs:file_tokens()
.url.site
cache context because because#type => link
/LinkGenerator
/Link
requires aUrl
object to be passed, andUrl
objects do not support relative URLs: they require fully qualified URLs. Those three cases are:template_preprocess_file_link()
\Drupal\file\Plugin\Field\FieldFormatter\BaseFieldFileFormatterBase::viewElements()
\Drupal\file\Plugin\Field\FieldFormatter\FileUriFormatter::viewValue()
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
@todo
s indicating so.This fixes 99% of the problem.
Comment #79
Wim LeersTest failures, yay! They show we have decent test coverage for this. Now updated the test expectations, this should now be green & RTBC'able again :)
Comment #80
mfbwe'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()
Comment #81
Wim LeersThanks!
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 ofcore/misc/druplicon.png
, thus already not reflecting reality.)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)ImageFormatter::viewElements()
: great catch! I can't believe I missed that one.ImageStyle::buildUri()
doesn't have anyfile_create_url()
, I think you meantImageStyle::buildUrl()
. This one I did not forget about, I explicitly didn't change it. Because:file_url_transform_relative()
. And that's in facht what #77 already did. For example:But, what would be valuable/useful there, is for
ImageStyle::buildUrl()
to referencefile_url_transform_relative()
just likefile_create_url()
does. So I did that.Comment #84
Wim LeersClearly 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.
Comment #85
Wim LeersBetter title.
Comment #86
mfbyes, 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.
Comment #87
Wim Leers\Drupal\Core\Entity\EntityInterface::url()
says . 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 frompublic://cat.jpg
tohttp://example.com/sites/default/files/cat.jpg
thanks tofile_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.Comment #88
mfbRead thru it quickly and the only problem I noticed is a typo in a @todo:
couldadd
Comment #89
mfboops :/
Comment #90
Wim LeersFixed.
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.
Comment #91
mfbTested 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.
Comment #92
catchSo 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.
Comment #93
mfbTo 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:
Comment #94
catchOh that makes sense and is fair enough.
One more question:
Shouldn't we always set this in KernelTestBase? Seems like a reasonable expectation that it would be set.
Comment #95
Wim LeersYes, and I was initially changing
KernelTestBase
. But, this could theoretically break some tests. So I didn't. Happy to move it though.Comment #96
catchThat 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.
Comment #97
Wim LeersThen 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.
Comment #98
catchOK that's fair. Just needs the change record and follow-up created then. Marking needs work for that.
Comment #99
Wim LeersFollow-up created: #2650362: Add test coverage to ensure KernelTestBase's simulated current Request has a hostname. Has patch.
CR created: https://www.drupal.org/node/2650374.
Comment #100
catchCommitted/pushed to 8.1.x, thanks!
Comment #102
Wim LeersHurray!
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.
Comment #103
Liam MorlandThanks very much, everyone!
Is this eligible for backport to D7?
Comment #104
tim.plunkettIn 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
Comment #105
mfbThe 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.. :)
Comment #106
catchRe-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.
Comment #107
catchSo #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.
Comment #109
Wim Leers#90 rebased against 8.0.x, with thanks to git's 3-way merging — I had to manually fix only one hunk.
Comment #111
catchCommitted/pushed to 8.0.x, thanks!
Comment #112
Wim LeersPublished 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!
Comment #113
cilefen CreditAttribution: cilefen commentedNice job everyone on this issue.
Comment #114
benjy CreditAttribution: benjy at PreviousNext commentedIt'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.
Comment #115
Wim Leers@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.
Comment #116
Wim LeersComment #117
Liam Morland@#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().
Comment #118
benjy CreditAttribution: benjy at PreviousNext commented@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.
Comment #120
fmeirelesfon CreditAttribution: fmeirelesfon commentedThe 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()"Comment #121
TR CreditAttribution: TR commentedWell 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.
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?
Comment #122
Wim Leers#121:
You can already do exactly what you want:
The docs even say this:
Comment #123
catchComment #124
TR CreditAttribution: TR commentedNo 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:
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.
Comment #125
benjy CreditAttribution: benjy at PreviousNext commentedThis 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.
Comment #126
mfbArguably 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.
Comment #127
Wim LeersYou'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.
Comment #128
catchThat seems like a reasonable option to fix the regression while avoiding mixed-mode warnings etc.
Comment #129
Wim Leersservices.yml
to include theurl.site
cache context. That'd be a simple solution, but…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 forUrlGenerator
(for generating non-file URLs). For non-file URLs, #2335661: Outbound path & route processors must specify cacheability metadata ensured theurl.site
cache context is bubbled when explicitly generating absolute URLs.As part of doing that, we could introduce
FileUrlGenerator
, a sibling forUrlGenerator
.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.
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)!
Comment #130
BerdirIt'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.
Comment #131
Wim LeersAlready 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.
Comment #132
Liam MorlandMy 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).
Comment #133
BerdirYes, 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.
Comment #134
cilefen CreditAttribution: cilefen commented@Liam Morland I posted a patch on the other issue - can you see if it makes any sense? I didn't actually test it...
Comment #135
catch#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
Comment #136
TR CreditAttribution: TR commentedWe 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.
Comment #137
catch@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.
Comment #139
catchComment #140
BerdirBut 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?
Comment #142
mfbI'd agree that CSS files should contain relative URLs in the aggregated CSS, as they do in Drupal 7.
Comment #143
Wim Leers@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 aabsolute === TRUE||FALSE
variable to theimage
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:
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
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 aKernelEvents::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.
Comment #145
Wim LeersOne more such bad assertion.
Comment #146
BerdirI 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.
Comment #147
benjy CreditAttribution: benjy at PreviousNext commentedI 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.
Comment #148
Wim Leers+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.Comment #149
BerdirWe 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)
Comment #150
Wim LeersOk, 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:
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?
Comment #151
Wim LeersShall 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?
Comment #152
Wim LeersDone.
Comment #153
Fabianx CreditAttribution: Fabianx as a volunteer commentedOkay, 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:
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
Comment #154
benjy CreditAttribution: benjy at PreviousNext commented+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.
Comment #155
Fabianx CreditAttribution: Fabianx as a volunteer commentedI 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.
Comment #157
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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).
Comment #158
Wim LeersYes, please.
Comment #159
andypostHere's a clone for d7 #2714515: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send
Comment #160
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRemoving backport tag - thanks for creating the followup issue.