File is inserted with absolute URL

Here is a quick patch for the ones looking for a solution to this issue.

Ran into this with 6.x when trying to link core media entities. I can confirm it is also still happening on 5.x as well.

@oldeb's patch fixes the issue. Happy to work on fixing the tests if it will help.

@johnnny83 looks like you are still assigned to this issue though.

First attempt at fixing tests.

Assigning myself for review, and setting the version this issue relates to as 6.0.x-dev, although I think we should backport it to 5.x for folks still using that version.

My main question about this issue is whether rendering the media/file URLs should always relative, or whether this is something that sites should be able to choose between this. This issue was originally entered as a "Feature request," but the issue description is more of a bug report.

For example, is there a scenario where Drupal media/file entities could be defined to have an external backend (such as AWS) where stripping the absolute path would cause those links to break? (To be clear, I'm not talking about plain URLs to external files, and this would be a pretty weird edge case.)

This issue also is related proposals about how media/file entity links should be rendered in ✨ Detect and strip base URL from pasted URLs to increase matching hits and support multilingual Needs work , 🐛 Media substitution plugin renders absolute paths Closed: duplicate , and (maybe?) 🐛 Cutting off /edit for media path breaks entity detection Needs work , so I'd like to consider this in the context of those.

In particular, 🐛 Media substitution plugin renders absolute paths Closed: duplicate seems like it's solving the same underlying problem (it calls out this issue in the description, too).

Thoughts?

My main question about this issue is whether rendering the media/file URLs should always relative, or whether this is something that sites should be able to choose between this. This issue was originally entered as a "Feature request," but the issue description is more of a bug report.

For me it appeared to be a bug since the generated links were not using the correct base url and were breaking links to media files.

For example, is there a scenario where Drupal media/file entities could be defined to have an external backend (such as AWS) where stripping the absolute path would cause those links to break? (To be clear, I'm not talking about plain URLs to external files, and this would be a pretty weird edge case.)

I don't know enough of how different backends handle this, but would assume contrib modules like S3 File System → or Azure Blob Storage File System → already provide a way to configure this.

I think the S3 module has a Drupal slack channel. Maybe running the change past someone there might give more insight?

For me this definitely is in category "bugs". Drupal uses relative paths everywhere, most notably the media module itself uses relative paths for links to its file entities too.

If other contrib modules like AWS need absolute paths, this has to solved in their scope.

If other contrib modules like AWS need absolute paths, this has to solved in their scope.

That's more or less the conclusion I was coming too, as well. Better to follow convention of the Drupal media module itself.

Thanks for the feedback, folks!

I'm going to mark 🐛 Media substitution plugin renders absolute paths Closed: duplicate as a duplicate of this issue (its patch is outdated and is also not as comprehensive as this one), and just want to consider any possible implications for this change on ✨ Detect and strip base URL from pasted URLs to increase matching hits and support multilingual Needs work . After that, I plan to merge this bugfix.

+++ b/src/Plugin/Linkit/Substitution/File.php
@@ -24,7 +24,7 @@ class File extends PluginBase implements SubstitutionInterface {
     $url = new GeneratedUrl();
     /** @var \Drupal\file\FileInterface $entity */
-    $url->setGeneratedUrl(\Drupal::service('file_url_generator')->generateAbsoluteString($entity->getFileUri()));
+    $url->setGeneratedUrl(\Drupal::service('file_url_generator')->generateString($entity->getFileUri()));
     $url->addCacheableDependency($entity);
     return $url;
   }

FWIW, a lot of this could be greatly simplified with $entity->createFileUrl(), which defaults to a relative path.

Bigger change for the patch, but easier code to maintain in the end.

Two notes:
* Core also switched to this in a several file/image formatters as part of #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it → , there was a bit of a backlash on that, but in contexts that I think are less likely to be an issue for this module (RSS feeds and other API-ish responses)
* generateString() returns a non-absolute link *if possible*. If you have an external storage like AWS, use CDN/asset domain or anything like that then you can absolutely still make that absolute and point to wherever you want. There has been a hook for that since forever and the streamwrapper also controls this. So the risk of this should be minimal.

[switching to relative URLs for files is] less likely to be an issue for this module (RSS feeds and other API-ish responses)

I can imagine a reasonably plausible scenario where absolute URLs would be wanted: Drupal backend that hosts static files with a decoupled frontend that links to those files in content served by the backend & run through a text format filter.

But I don't think that it should be the responsibility of the Linkit text filter to support that. For folks that really do need absolute URLs in the context of what a Drupal text format filter renders, one solution could be to use https://drupal.org/project/pathologic, which supports specifying an absolute path.

So I agree that the impact of this change to relative URLs is manageable for folks who still want absolute URLs.

Somewhat related to #20 above, this patch combined with the patch in https://www.drupal.org/project/linkit/issues/2712951 ✨ Linkit for Link field Fixed does not allow for linking directly to files. I was about to post on 2712951 but then reinstalled without the patch from #13 – linking to files totally works now!

Thanks to everyone for your contributions here. So much amazing progress on this module already this year.

The attached patch uses Berdir's suggestion from #19 for simplifying the generation of the URL. Interdiff from the previous patch is included. This functionally checks out for me, and the relevant automated tests pass. Since Berdir said "Bigger change for the patch, but easier code to maintain in the end," I want to double-check that I'm not misinterpreting, since this seems not much of a "bigger change."

Separately, following up on:

does not allow for linking directly to files. I was about to post on 2712951 but then reinstalled without the patch from #13 – linking to files totally works now!

Thanks for the feedback, jds1. Can you elaborate a bit? I'm confused! What I'm hearing is that the proposed patch in this issue is preventing you from linking directly to files, and the implication, since you mention ✨ Linkit for Link field Fixed , is that you're doing so from a link field using the Linkit autocomplete widget there. That isn't my result when I test this with the existing patch (I am using the 6.0.x branch, however, which includes the Linkit for Link field): I see an option to either link to the File entity or the Media entity from the Linkit autocomplete (in both CKEditor and in the Link field widget). If you're getting a different result, can you provide steps to reproduce? Thanks!

+++ b/src/Plugin/Linkit/Substitution/File.php
@@ -24,7 +24,7 @@ class File extends PluginBase implements SubstitutionInterface {
     $url = new GeneratedUrl();
     /** @var \Drupal\file\FileInterface $entity */
-    $url->setGeneratedUrl(\Drupal::service('file_url_generator')->generateAbsoluteString($entity->getFileUri()));
+    $url->setGeneratedUrl($entity->createFileUrl());

I meant a bigger conceptual/per line change. The previous patches essentially just changed generateAbsoluteString to generateString, this removes the file url generator service (or hides it within the method we call now in reality).

the result is the same and the patch file size is the same or even smaller, yes.

Alright, we now have relative URLs in rendered file paths!

If jds1 still has a scenario where this causes issues linking directly to files (#21), we can address that when more information is available.

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

Re #25, see 🐛 Direct URL to media file entity does not work because relative URL does not pass URL path validation Fixed , which seems to be a regression caused by this change.