Repeated file_usages when listing them

Can you please provide the steps to reproduce the issue?

Also should this be tested on the latest version of D8? 8.0.1

Steps to reproduce the issue reported:

:

1. simplytest.me + devel module
2. generate 50 nodes of content for article
3. https://rmnoc.ply.st/admin/content/files/usage/3

After patch steps :

1. simplytest.me + repeated_file_usages-2632372-8.patch + devel module
2. generate 50 nodes of content for article
3. https://rmnon.ply.st/admin/content/files/usage/1

Changed status to RTBC

I think we should improve Drupal\file\Tests\FileListingTest to test for this.

Also I think we need to consider an upgrade path here.

I'd like to give #13 ago if there is some time for researching how.

Too big for me.
Issue summary updated.

+++ b/core/modules/file/file.install
@@ -120,3 +120,17 @@ function file_requirements($phase) {
+function file_update_8001() {
+  // core/modules/file/config/optional/views.view.files.yml was changed in
+  // https://www.drupal.org/node/2632372 to change group_by to true.
+  $config_factory = \Drupal::configFactory();
+  $view = $config_factory->getEditable('views.view.files');
+  $displays = $view->get('display');
+  $displays['page_2']['display_options']['group_by'] = TRUE;
+  $view->set('display', $displays);
+  $view->save(TRUE);

Lets make this a bit more defensive. Check if the view exists, check if the setting you want to check exists and is actually set to FALSE right now.

If not, do not change/save the view.

This is default configuration, users might have deleted or changed it.

This is neither a "new development" nor a "disruptive change".. We should not wait for 8.3.x to fix this.

Maybe it's just lack of information on my part, but looking at http://cgit.drupalcode.org/drupal/tree/core/modules/file/config/optional..., I still see that 8.2.x has that group_by still set to false. If we change it to true on the 8.1.x patch, shouldn't we do the same in the 8.2.x patch? And also add the file_update_8001() function? There will be people upgrading 8.1.x sites to 8.2.x sites, after all.

OK. Let's forget about the 8.1 patch now.. It will soon be irrelevant anyway.

However, reading the 8.2 patch, the only non-test change is to remove a join with file_managed. I don't see how that relates to your answer of "we use the relationship".

At this point, the patch should be applied to 8.3.x, and then patched to 8.2.x.

Let's see if the testbots are ok with it.

Testbots are OK, and so is my test on a real system..

This is definitively RTBC in 8.3.x and 8.2.x (same patch applies in both at this time).

What happens if there are views out there relying on the automatic join?

Indeed. It may be safer, and way more appropriate to use the solution in the 8.1.x patch.

Re-rolled the 8.1 patch in #30 for Drupal 8.4. This uses only the aggregation and then runs file_update_8001 to update the config for existing views.

No interdiff, as it is identical to #30.

I tested on Drupal 10.1.x, standard install and this is still relevant.

have closed #2560937: Multiple file usage records as a duplicate and adding credit.

Still needs to write tests

This could use steps to reproduce. Not sure I'm seeing an issue.

Also as mentioned needs tests.

A while back I added reproduce code to the duplicate issue at #2560937: Multiple file usage records, now copying it to this issue

Thanks @mfb confirmed I see the issue now.

There was a discussion around two approaches in the issue. One patch was for 8.1.x and another one was for 8.2.x

The one that was for 8.1.x #24 #30 #45 was updating a views configuration and adding aggregation.
I think in #42it was discussed that this is not the good approach. I quite agree with that.
It is good to remove un-necessary join rather than adding aggregation.

Now patches for 8.2,x, 8.6.x mentioned in #30and #45 comes to picture. I feel that this is the correct approach.
Prefereed solution:
Remove the additional join happening in hook_views_data of the FileViewsData.php class.

The test cases added in #45 seem a bit confusing, so I've removed that test case and added a new simple one to validate the error. Attached test-only patch as well. Since we are not updating any views this time, I think we won't need any update hook or upgrade path test.
I've updated issue summary accordingly focussing approach #2 which is about removing additional join in the hook_views_data.

#59 re-roll is missing a couple of changes mentioned in the #45
Reportedly FileUsageTest.php file's changes are not included in the re-roll.

Opps, putting back the views related changes.
Didn't realised that those changes might break the view's functional tests.
Not uploading the test only patch since test-only patch is failing for the new test case only.

Confirmed the issue using the snippet in the issue summary and the patch in #67 solves the issue.
#63 shows the tests fail as expected.

I think previous failures were un-related.
Triggering run again for 11.x
Let's see how goes 🤞

Tried to fix the issue.
To fix the issue, I had to update the view.
Which leads me to think that can there be any regression on the existing views because of the change?
Do we need to consider any upgrade path here?

+++ b/core/modules/file/tests/modules/file_test_views/config/optional/views.view.test_file_to_node.yml
@@ -187,25 +132,90 @@ display:
+        fid:
+          id: fid
+          table: file_managed
+          field: fid
+          relationship: none
+          group_type: group
+          admin_label: 'File usage'
+          entity_type: file
+          entity_field: fid
+          plugin_id: standard
+          required: true
+        file_to_node:
+          id: file_to_node
           table: file_usage

I had to add this whole relationship.
Which makes me think do we need to consider any upgrade path here?

Looking forward to see opinions from other folks.

Updating another view which was causing issues in relationship.
Hiding the patch in #73.

This was previously RTBC and since the update needed was for a test view think it's safe to go back to RTBC!

Re: #73, an upgrade path should not be necessary as long as the views being changed are only used for testing. At the moment, there are no changes to any views config, so an upgrade path is not necessary.

I manually tested this, and it worked as designed, without any updates to existing config.

It did however require a cache clear to work.

Should we add an empty post update hook to trigger the cache invalidation that resets the views config?

Or should we leave it as is knowing that the missing cache clear doesn't break anything, its just the bug doesn't get fixed until it is cleared

If the standard is to do an empty post_update hook think it wouldn't be terrible to add here.

Added an empty post_update hook.

There is a literally random failure on this patch, but it seems to work. I just don't understand the "why": Removing the built-in relationship between file_usage and file_managed (which seems like a useful thing to define) only to add this relationship back in the view itself?

If it really does make sense to remove this from the views data, then the above comment should be updated too:

Provide field-type-things to several base tables; on the core files table ("file_managed") so that we can create relationships from files to entities, and then on each core entity type base table so that we can provide general relationships between entities and files.

Removing this from views data seems problematic if every custom view that relied on it needs to be updated, so it presumably would need a change record?

By the way, I noticed that media was never added here as one of the core entity type base tables..