File usage view causes fatal error for a file with 0 usage

Is this still an issue as of today? I pulled down the latest code and tried to duplicate the problem. Can you provide a screenshot?

I just did a git pull on 8.x and started from scratch with a new database. I was easily able to reproduce the issue. Screenshot attached.

Steps to reproduce:
1) Create a new Article, includiing text AND a picture. Save the article
2) Edit the article - remove the picture (click the Remove button). Save the article
3) Go to .../admin/content/files
4) Click on "0 places"
5) See error mesage

Added the VDC tag, otherwise we will not see/find this :)

Yeah, it has an array of empty things like that because the base table for that view is file_managed with a left join to the file usage table. Ideally this view would use an inner join, problem solved, but we can't just change the views data like that.

So we could just fix it like this for now as I'm not sure if we have a better way for now, as we will get a record with empty values. It would be nicer to fix this on the query level though.

Sorry, that didn't apply.

Here is a new patch that contains that fix (probably good anyway), and a removal of the implicit relationship between file_managed and file_usage. As mentioned above, this is resulting in always using a left join, but for the single file usage view, we want this to be inner.

So if we switch to just using actual relationships, we get this flexibility.

Would be grateful if people could test this and confirm.

This new issue queue format is killing me :)

I really like to kill implicit relationships.

Shouldn't we add a test for this?

I think the only feasible way to add tests for this is to just test the default file views. This should cover that. Here is a patch with just new tests and with fixes.

I was surprised to find no coverage at all for the default file views, not sure what happened when they got in? :)

No, I think we're good; the whole point of using the relationship instead of an implicit join (in views data) is that we can change this, and each view requires a different join. Which we can now easily do (in the UI this translates to the 'require this relationship' checkbox).

11: 2120839-11-PASS.patch queued for re-testing. - Random UrlTest fail.

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.php
   @@ -123,7 +126,10 @@ public function preRender(&$values) {
        foreach ($values as $value) {
   -      $entity_ids_per_type[$this->getValue($value, 'type')][] = $this->getValue($value);
   +      if ($type = $this->getValue($value, 'type')) {
   +        $value = $this->getValue($value);
   +        $entity_ids_per_type[$type][$value] = $value;
   +      }
        }
   

   Could this work like this?

   foreach ($values as $value) {
     if ($type = $this->getValue($value, 'type')) {
       $entity_ids_per_type[$type][] = $this->getValue($value);
     }
   }
   

2. We already have test for file listing. There is no need to add another one. We should just add coverage for this kind of errors.

   See: https://api.drupal.org/api/drupal/core%21modules%21file%21lib%21Drupal%2...

Ok, merged those assertion into the current FileListingTest, thanks for pointing that out! Couldn't find that initially for some reason :?

Also made the change to EntityLabel::preRender

Looks good to me. Let's wait for test bot. RTBC if green as far as I'm concerned.

Pass patch is good.

+1 for the test coverage!

Yay! :) good job!

Reading the changes to the view, my assumption was the same as Dave's, but I tested manually and indeed files with 0 usages are showing up.

Thanks for both the bug fix, and the test! :)

Committed and pushed to 8.x.

Thanks.

Why do people not trust us views guys?! WE KNOW VIEWS... ;)

Oh, we know! :D I just sometimes find weirdness when manually testing things, since I don't know Views as well as you do. :)

Fix media tag.