Direct URL to media file entity does not work because relative URL does not pass URL path validation

Potentially a new suggestion plugin for handling media absolute url's.

Thanks for explaining the problem introduced by switching to relative URLs and for the new suggestion plugin. Assigning myself for review (though I'm not yet sure that this is the best way to achieve this...)!

Not sure I fully understand, could this be related to 🐛 Cutting off /edit for media path breaks entity detection Needs work ?

@Berdir no, I don't think so.

We've been using the patch from ✨ Linkit for Link field Fixed for a long time now, together with the "Direct URL to media file entity" substitution type.
After switching to Linkit 6.0.0-RC1, which contains both the Linkit for Link Field functionality plus the changes from ✨ File is inserted with absolute URL Fixed , the media link substitution has no effect any more, because the links are now relative and therefore don't pass the validation in PathValidator::getUrlIfValid() since the link URLs don't refer to Drupal routes.
Thus, links that used to point to the media file (/sites/default/files/YYYY-MM/filename.ext) now link to /media/1234.

Adding another suggestion type for media that generates absolute URLs (as @andy_w suggested) has the drawback that all sites that have been using a similar setup as pointed out above will need to update that setting. I know that Linkit's new link formatter has never officially existed before, but given the fact that its issue has lived so long, I'm quite sure that many sites will be affected.

@andy_w's second option sounds more promising to me: if you compare the functionality of the Linkit filter vs. the field formatter, the former doesn't do any URL validation at all. So why do it in the formatter?

Didn't mean to change the status or unassign the issue from @mark_fullmer, sorry. Changing the version to 6.0.x though, as it is also affected.

Here's a quick POC for the URL validation skipping.

Need to avoid double-encoding of the link URL.
I'm not very fond of the direction in which this patch is going; the whole conversion of a GeneratedUrl into a Url object is a bit of a mess. I wonder why the substitution plugins don't return Url objects in the first place, but this is most likely out of scope of this issue.

#8 did fix direct downloads for me, I agree that the fix is weird.

I'm not very fond of the direction in which this patch is going;

I agree that the fix is weird.

Since the 'problem' here is limited to media entity URLs, could we scope the solution to conditionally handle those differently than the rest of the URLs?

I see. Yes, reliably converting back from a string/GeneratedUrl to a Url object is basically impossible if you consider Drupal installations in a sub-folder/path, see 🐛 \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links Active .

I do agree that allowing to return URL would be the most sensible thing to do, but that's technically a BC break if someone were to call those substitution plugins themself, so that's up to @mark_fullmer to decide.

The good thing is that the issue referenced above is now committed, so we could in fact change to use \Drupal\Core\File\FileUrlGeneratorInterface::generate() and then don't have to worry converting that back into a URL object.

The other option I guess is to revert the relative file path issue and postpone it on the API change and end-end test coverage for link fields and media references.

The good thing is that the issue referenced above is now committed, so we could in fact change to use \Drupal\Core\File\FileUrlGeneratorInterface::generate() and then don't have to worry converting that back into a URL object.

The attached patch is just intended as a demonstration of the usage of FileUrlGeneratorInterface::generate(), as a replacement for converting back to the URL object.

As for the question about whether the substitution plugins return a URL themselves, I'm inclined to do that, despite it being a BC break. Will think on this further.

Raising the priority to "Major" as this is currently making direct media URLs not work.

@mark_fullmer, in case you decide to make the change and return URLs in substitution plugins, here's a first attempt at doing so. I wasn't sure if it belongs here or in a separate issue, but since it fixes the original issue, I'm posting it here. Tried to update the tests as well.

The last submitted patch, 14: 3354873-use_url_objects-14.patch, failed testing. View results →

Forgot to update the LinkitFilter as well

The last submitted patch, 16: 3354873-use_url_objects-16.patch, failed testing. View results →

The last submitted patch, 18: 3354873-use_url_objects-18.patch, failed testing. View results →

This should fix all failing tests except one, which is related to a URL-encoded colon in the "/vfs:" protocol in \LinkitFilterEntityTest::testFilterFileEntity. Not sure how this test really works, so maybe somebody can help with that one.

The last submitted patch, 20: 3354873-use_url_objects-20.patch, failed testing. View results →

Thanks so much, folks. So glad to find this patch when I came across this bug. Working as advertsied, no idea about the failing test, sorry! +1!

This should fix all failing tests except one, which is related to a URL-encoded colon in the "/vfs:" protocol in \LinkitFilterEntityTest::testFilterFileEntity. Not sure how this test really works, so maybe somebody can help with that one.

I consider this a non-problematic byproduct of the change to using \Drupal\Core\File\FileUrlGeneratorInterface::generate() instead of $file->createFileUrl(), and I think it's acceptable in the context of the failing test to resolve the problem by using urldecode() to normalize the output.

The attached patch does that. Including the interdiff from #20 to clarify.

This change introduced a compatibility break by changing an interface's method return type. This breaks media_entity_download ( ✨ Compatibility with Linkit 6 Needs work ) and could break other modules or custom implementing \Drupal\linkit\SubstitutionInterface.

https://git.drupalcode.org/project/linkit/-/blob/ae11deca06c3312e23a0f40...

Do we want to reopen this issue or file a new one?

Yes, this seems like a very problematic BC break in a patch release, we IMHO need to keep support for GeneratedUrl responses and trigger a deprecation for the next major version or so?

This change introduced a compatibility break by changing an interface's method return type. This breaks media_entity_download

Yes, this is exactly what Berdir anticipated in https://www.drupal.org/project/linkit/issues/3354873#comment-15026015 🐛 Direct URL to media file entity does not work because relative URL does not pass URL path validation Fixed

I'll make a new issue to revert this change and instead add deprecation notices for the next major version. The issue with using a direct URL for media will return; I'm leaning toward doing a short-term fix along the lines of https://www.drupal.org/project/linkit/issues/3153482#comment-15236610 🐛 Cutting off /edit for media path breaks entity detection Needs work

What I meant is that you support both where you call that method, so you do an instanceof check of the return value, if it's GeneratedUrl, then trigger a deprecation message and do the old code, there is no strict return type on the method/interface, so it should IMHO only fail when being used as the wrong thing.

The only problem is then BC if someone directly uses that API and calls the plugin themself.

What I meant is that you support both where you call that method, so you do an instanceof check of the return value, if it's GeneratedUrl, then trigger a deprecation message and do the old code

Makes sense. Thanks for clarifying! I'll proceed along those lines.

This also introduced a caching issue - LinkitFilter::process does this:

            // The processed text now depends on:
            $result
              // - the generated URL (which has undergone path & route processing)
              ->addCacheableDependency($url)

That $url is not an instance of CacheableDependencyInterface which means max-age gets set to 0 and breaks cacheability for the entire page.

Added 📌 Add cacheability tests Active

I'll make a new issue to revert this change and instead add deprecation notices for the next major version.

Has this been done? Given the BC and cache issues it'd be good to get this reverted and a new minor out ASAP

Given the BC and cache issues it'd be good to get this reverted and a new minor out ASAP

I agree with the sense of priority, and am planning to work on this today. I have re-opened this issue and set it back to "Needs work,". Thanks for creating the separate issue for adding cacheability test coverage!

The attached patch provides backwards-compatibility for Substitution plugins that return a GeneratedUrl object, both for the text format filter and the field formatter.

I tested these changes with the current version of Media Entity Download, confirming that the legacy subsitutions work. (There is an issue there to make the module work with Url, at ✨ Compatibility with Linkit 6 Needs work .

This change also addressed the uncacheability reported in #33 above by only adding the cacheable dependency if the URL is of type GeneratedUrl. This does not yet include test coverage, which can either be added here or subsequently via 📌 Add cacheability tests Active .

Setting to "Needs review" for community input.

1. +++ b/src/Plugin/Field/FieldFormatter/LinkitFormatter.php
   @@ -108,6 +108,13 @@ class LinkitFormatter extends LinkFormatter {
   +        $implementing_class = get_class($url);
   +        if ($implementing_class === 'Drupal\Core\GeneratedUrl') {
   +          /** @var \Drupal\Core\GeneratedUrl $url */
   

   why not $url instanceof GeneratedUrl, then you don't need the @var?

2. +++ b/src/Plugin/Field/FieldFormatter/LinkitFormatter.php
   @@ -108,6 +108,13 @@ class LinkitFormatter extends LinkFormatter {
   +          $url = $this->pathValidator->getUrlIfValid($url->getGeneratedUrl());
   +          @trigger_error('Drupal\Core\GeneratedUrl in Linkit Substitution plugins is deprecated in linkit:6.0.1 and must return Drupal\Core\Url in linkit:7.0.0. See https://www.drupal.org/project/linkit/issues/3354873', E_USER_DEPRECATED);
   

   I don't think that will work correctly when you have things like language prefixes or drupal installed in a subdirectory. See 🐛 \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links Active .

3. +++ b/src/Plugin/Field/FieldFormatter/LinkitFormatter.php
   @@ -120,11 +127,13 @@ class LinkitFormatter extends LinkFormatter {
   -        // file substitution.
   +        $cacheable_metadata = BubbleableMetadata::createFromRenderArray($item);
   +        if ($implementing_class === 'Drupal\Core\GeneratedUrl') {
   +          // Add cache dependency if the substitution uses legacy GeneratedUrl
   +          // This can be removed in 7.0.0 per
   +          // https://www.drupal.org/project/linkit/issues/3354873 .
   +          $cacheable_metadata->addCacheableDependency($generated_url);
   +        }
   

   here I'd go with instanceof CacheableDependencyInterface, then you can probably even keep that as Url might at some point support that too, see 🌱 [Meta] Allow StreamWrapper's to provide cacheability metadata Active

4. +++ b/src/Plugin/Linkit/Substitution/Canonical.php
   @@ -21,6 +21,11 @@ class Canonical extends PluginBase implements SubstitutionInterface {
       */
      public function getUrl(EntityInterface $entity) {
   +    $implementing_class = get_class($entity);
   +    if ($implementing_class === 'Drupal\Core\GeneratedUrl') {
   +      @trigger_error('Drupal\Core\GeneratedUrl in Linkit Substitution plugins is deprecated in linkit:6.0.1 and must return Drupal\Core\Url in linkit:7.0.0. See https://www.drupal.org/project/linkit/issues/3354873', E_USER_DEPRECATED);
   +      return $entity->toUrl('canonical')->toString(TRUE);
   +    }
        return $entity->toUrl('canonical');
   

   I don't get this change, that seems impossible? The entity is just an entity, only the return value can be Url or GeneratedUrl, not the input?

+++ b/src/Plugin/Field/FieldFormatter/LinkitFormatter.php
@@ -120,11 +127,13 @@ class LinkitFormatter extends LinkFormatter {
+        if ($implementing_class === 'Drupal\Core\GeneratedUrl') {

Why not just check instanceof CacheableDependencyInterface::class

#38.1: done.
#38.2: the patch just restores the behavior we had before, so if this is a problem, it should probably be handled in a separate issue?
#38.3: done, tried to address this in a more generic way.
#38.4: agreed, I reverted that change.

I also extended the return type hint in getSubstitutedUrl() to reflect the fact that GeneratedUrls are still supported.

> #38.2: the patch just restores the behavior we had before, so if this is a problem, it should probably be handled in a separate issue?

Right, I missed that, and it's just the BC layer now, so great that we get rid of this :)

I'll let others confirm who actually have updated and did run into problems, but this looks good to me.

@40 works for me also against 6.0.1 (on D9 )

Thanks for the input, everyone. The backwards compatibility has been included in follow-up releases 6.0.1 → (Drupal 9.5->10.0) and 6.1.1 (Drupal >=10.1).

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

Unfortunately this still doesn't seem to work with Linkit 6.1.2 either by itself or with Media Entity Download 8.x-2.2 enabled.

Every option presented in the matcher settings for 'URL substitution' still only substitutes 'media/[id]'.