File usage view breaks if an entity uses a file that has no canonical link template

Discussed with @xjm and @catch. We agree that this is major because if an entity don't have a canonical link template and you attach file fields a core provided view will break.

I've been able to reproduce the bug, patch attached.
Thanks for the example module, it helped to speed up the process.
After patch screenshot attached as well.

The code looks good.
I tested on simplytest.me, and can confirm that it works.

Instead to get all templates first and then check, we also have hasLinkTemplate() for entity.

You're totally right, thanks for the tip.
Here is the tested patch.

Patch #10 works for me, you're right getEntityType() is not needed as well.

Added WebTest pretty much following the directions outlined by @berdir in #12

Interdiff is the test-only patch

+++ b/core/modules/file/src/Tests/FileListingTest.php
@@ -145,6 +146,48 @@ function testFileListingPages() {
+      'name' => $entity_name,
...
+    // Linked entity name should not be displayed if there is no link template.
+    $this->assertNoLink($entity_name, 'Linked entity not added to usage listing');

I'm wondering whether we should add a positive assert as well, something like assertText($entity_name) ? I always like those more a little more. Shouldn't hold up commit though, rest looks good to me.

Good idea @swentel
Added the positive assertion for the entity name. Had to replace the test entity type used with another to allow the entity name to display.

Stumbled upon this... It looks like we need to explicitly set make_link to FALSE if there is no "canonical" template. Otherwise, in case of multiple usages, no-canonical entity's name is linked with a URL from a canonical-entity.

Uploaded a patch with 2 usages (canonical/non-canonical) as test-only example.

Correct patch.

@mbovan Good catch on output with multiple usages on the file. Can you upload an updated test-only patch to show a test failure in that scenario?

Setting to Needs Work since I think that updating the tests to show that failure is relevant to the issue.

Actually, I got an exception while working on a client project where the fix is a patch from #21.

On a clear 8.1.x branch, I cannot reproduce it... Also, seems like node usage #19 patch is not failing on testbot, so I'm removing my patches and pointing to #17 as the latest valid patch, for now...

This solves the problem, seemed to be quite reasonable.

Is #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation an alternate solution?

Also if we #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation then this issue is fixed for files - but not for other entities without a canonical template.

Yes thats right. If I remember correctly, I faced this issue in paragraph module as well as there was no canonical for that.

+++ b/core/modules/views/src/Plugin/views/field/EntityLabel.php
@@ -105,8 +105,13 @@ public function render(ResultRow $values) {
+      if ($entity->hasLinkTemplate('canonical')) {
+        $this->options['alter']['make_link'] = TRUE;
+        $this->options['alter']['url'] = $entity->toUrl();
+      }
+      else {
+        $this->options['alter']['make_link'] = FALSE;
+      }

I don't think we should be checking for a link template here. I think we need to be calling $entity->toUrl() and catching UndefinedLinkTemplateException and EntityMalformedException and in this instance setting make_link to FALSE.

This is because it is possible to provide URLs without link templates - see #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation and also the uri_callback annotation.

I adjusted the patch with alex hints

Patch looks good.

Looks good for me as well.
Marking as RTBC

+++ b/core/modules/views/src/Plugin/views/field/EntityLabel.php
@@ -105,8 +107,14 @@ public function render(ResultRow $values) {
+      try {
+        $this->options['alter']['url'] = $entity->toUrl();
+        $this->options['alter']['make_link'] = TRUE;
+      } catch (UndefinedLinkTemplateException $e) {
+        $this->options['alter']['make_link'] = FALSE;
+      } catch (EntityMalformedException $e) {
+        $this->options['alter']['make_link'] = FALSE;
+      }

One small thing we could clean up here is that this does not follow our coding standards for control structures -- see: https://www.drupal.org/coding-standards#controlstruct

This could be fixed on commit (so leaving RTBC), but also would be easy to clean up in a patch. :)

Just fixing the coding style issues

1. +++ b/core/modules/file/src/Tests/FileListingTest.php
   @@ -145,6 +146,55 @@ function testFileListingPages() {
   +    // Entity name should be displayed, but not linked if there is no canonical
   +    // link template.
   

   This comment is not quite right - it is if Entity::toUrl throws an exception.

2. +++ b/core/modules/file/src/Tests/FileListingTest.php
   @@ -145,6 +146,55 @@ function testFileListingPages() {
   +    $this->assertNoLink($entity_name, 'Linked entity name not added to file usage listing.');
   

   This test will break once #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation lands... which indicates we should have a test that is not tied to file entities. Maybe using the entity_test entity and not having a canonical link.

1. Fixed

2. We discussed that with alexpott and came to the point that #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation will not break the test, because the test covers the relation from a file to an entity. Not the other way round

So IMHO we can move this fast to RTBC

@chr.fritsch yep #34.2 was wrong - sorry everyone.

Then we are done here :)

Committed bbcd3bd and pushed to 8.1.x and 8.2.x. Thanks!

diff --git a/core/modules/views/src/Plugin/views/field/EntityLabel.php b/core/modules/views/src/Plugin/views/field/EntityLabel.php
index 3c7ccfb..967fd52 100644
--- a/core/modules/views/src/Plugin/views/field/EntityLabel.php
+++ b/core/modules/views/src/Plugin/views/field/EntityLabel.php
@@ -110,9 +110,11 @@ public function render(ResultRow $values) {
       try {
         $this->options['alter']['url'] = $entity->toUrl();
         $this->options['alter']['make_link'] = TRUE;
-      } catch (UndefinedLinkTemplateException $e) {
+      }
+      catch (UndefinedLinkTemplateException $e) {
         $this->options['alter']['make_link'] = FALSE;
-      } catch (EntityMalformedException $e) {
+      }
+      catch (EntityMalformedException $e) {
         $this->options['alter']['make_link'] = FALSE;
       }
     }

Fixed coding standards on commit.

Fix for use of current tag. Sorry for using the wrong one.