Create simple file listing under admin/content/file

Sounds like a view with some bulk operations. I think discussing/perfecting the view in itself is valuable even if we don't immediately add bulk operations. Who wants to take this on?

See also: #1823450: [Meta] Convert core listings to Views

In particular:
#1851086: Replace admin/people with a View
#1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations

Great Issue! It is very handy function for site administrators.

Please have a look to this related projects: https://drupal.org/project/auditfiles and https://drupal.org/sandbox/andyceo/1131236

From this projects you can find that main problem with files is sync between database and file system.

If I can help with something, please let me know.

Great start! Here are a few quick screenshots.

1. No files available. This looks like would need a message or something:

1a. This is how it looks like when the content list is empty:

(We'll not have an add file action here, but an empty table with a message looks like in order :) (Also operations :)

2. Once I had a few files:

(Can we somehow get Views to put a limited size preview into the first column above / below the file name if the file is previewable (with image styles)? If that does not apply, only the filename would show. Would that be too much of a jumble?)

We could check whether that file is an image, nice idea! That would looks more like a media library. What do you think about a table and a grid style? What opertations would make sense? File name (contains), mime type, file size and uploaded date or should we expose every column?

Oh, seems like we already have a mime type and status filter.
Gábor, which patch did you use? :o

The two patches were posted with only 16 minutes between them :) I used the first one, did not expect such a quick update.

#8: 2005166_8.patch queued for re-testing.

Nice idea!

Okay that's a good point. A contrib module could provide another view which provide a grid view.

I though that this was the whole point of converting admin listings to view: admins can edit them and add/remove fields at will - we simply give them something to start with and only account for what novice users would expect out of the box ;)

Great job so far everybody!

Looking at #9 as well. I think its strange that the node overview has the table shown (with the message in the table) but the file does not. I know we are not very consistent about this elsewhere either. Showing the table seems to inform the user more about what will be available here once there are files.

Also, the table shows up very close to the exposed filters. Does the node table have custom CSS to work around this?

Do we want to put in bulk operations or is that not a good idea given you are supposed to operate on these files via the entities they are attached to? Maybe we want to add some help text to that effect then? (I think the usage count would also need some explanation in there).

There is a setting in the table settings, "Show the empty text in the table".
Or add empty_table: '1' to the export under the table settings.

Yeah we probably need tests as well.

I think we should make it empty instead of displaying "N/A" - what do you think? I imagine if I would have lots of documents and the thumbnail column is always "N/A". It would be really cool if the thumbnail column only appears if there is an image in the table but that is not possible with Views, right?

I also think that we shouldn't add bulk operations as core relies on fields to take care about files. What kind of operations could we support after all?

I just created a node with files (and images) and right after the node was saved I got three files in the files overview but after deleting the node the file overview still shows the three files. Why are the files not deleted?

Is is the normal behavior that I get redirected to a non-overlayed page after submitting the exposed filters?

A cool feature would be to see *where* a file is used and not only how often...

Is showing where it is used making this table too busy or too slow or to wide or all of them? :) If we want to make the usages appear in a different table, maybe we want to have a View with an argument and the usage count link to that view with the proper file ID filled in, so it can look up usages for that file only?

Yeah! The File entity does not display the entity ID but it displays the entity label linked to the entity URI. I think this is cleaner than display the entity ID. Why do you display the module and what is the module? It's not the module which provides that file type because it would be file in every case.

We are getting there! Some notes:

- The clickable number provides very little click target to get to the usage list. Can we make it '1 use' or something like that?
- We should mark a few columns as less important for responsive tables, so we can see this in smaller screen spaces in a condensed view.
- The usage screen is *very* technical with the entity name, id, etc. Agreed with @yannickoo there, but I see the technical limitations. Maybe we just want to include the full entity URL as clickable? (Also improves click target) vs. just the entity name and id.
- Also a preview of the file (if possible) and the file name would be great to display on the usage table in views header text. This should be possible with tokens(?) - so we first see what are we getting info about and then the usages.

Views is so powerful :)

BTW current screenshots (admittedly the filename was in the page title already, but overlay makes it hard to recognise that :D)

Do we have to fix that behavior that after submitting the filters we get a non-overlayed page?

@yannickoo: existing bug at #1116326: Support admin overlay in exposed forms.

Gábor what do you think about adding more filters?

Does anybody of you know whether a "replace file" functionality will be added to core?

I don't think we should add or anticipate more features here. Like replacing files. We already direct people to find out where the file is used, they can go edit there. This can be extended by contrib. I could personally use more filters and ordering options on columns, but there can be any number of complexity for filters. I can imagine people wanting to search for files above certain sizes to clean up big junk files or other similar things, but they can now customize this. Its all views :D So we don't need to tailor this view for very specific use cases. Maybe one or two more filters would be useful, but it would quickly get confusing if we overdo it IMHO.

Good point - I totally agree with that!

Showing detailed usage information is out of scope for this listing. If anything let's just show a usage COUNT() in a column. I'm not really happy about adding images either. We're trying to make a poor-man's media library with this View which was against the intentions that I had for this. It would be easier just to keep it simple with a table display and not trying to 'display' any files, and leave the nicer view for contrib and admin/content/files/thumbnails.

i am against adding a thumbnail as well..the usage thing looks good though

I too think that this should be kept minimal. I don't mind the thumbnail because it's useful when people start naming files image1.png image2.png, etc, but if digging through the file usage shows admins in which nodes (title - not entity ID) the file is being used, then I could live without it.

The cool thing about these pages is that they are views now and admins can tweak them as they see fit. What I would like to see once we are finished with converting all these pages to views is a "Edit this display" link (perhaps at the bottom right of the table) directly linking to the view itself in edit mode.

...

What I would like to see once we are finished with converting all these pages to views is a "Edit this display" link (perhaps at the bottom right of the table) directly linking to the view itself in edit mode

thats what contextual links would do if they where not removed for admin views..

Yeah, do you have any idea why they were removed in the first place re patrida (perhaps link to a related issue/comment)?

There is a setting to not show them in the UI. It was turned off at webchick's request for the admin/content and admin/people views.

Also, if you have the ability to edit an admin view, you probably know how to get to the Views UI.

I think the usage information is important. Otherwise this is just a page listing things with no way whatsoever to act on them. You cannot act on files themselves, you can only act on them through their "holder entity" essentially. You cannot remove, edit, whatever files direct. So if there is no information and way to lead people to the place where you can act on the file, there is not really a point in having this list I think. All other admin content lists let you act on things, they are not just "here, this is data on your site, but good luck trying to figure out where you can do something with it".

#51 +1

Maybe we can have 2 displays, one table listing and one thumbnail grid to preview images?

Also, if you have the ability to edit an admin view, you probably know how to get to the Views UI.

I know, but having a "Customize this display" link available that takes you directly to edit mode spares you from 4-5 of extra clicks and the need to search through the list of views (some sites have quite a few and #1475204: [META] Provide a generic search/filter UI interface pattern for listing pages is not moving).

Please open a dedicated issue for that. Both of the 2 views in core have contextual links turned off, and this should follow suit.

opened #2020265: Bring back contextual links for admin views

Thanx ;)

Testing this it does not look any different visually than it did before. Let's figure out how to make the usage info more useful.

@Dave: I'm personally fine either way to push arguing about a thumbnail into a followup issue and remove that column for now.

@slashrsm

I think this needs a link in the menu as well. Otherwise it's not discoverable enough.

Agree. I was planning to propose another view, that would display list of usages for a given file. We could link to that listing from this view. What do you think about that?

That's not optimal from a UX perspective. It should just be another column in this view. We could remove one as well, for instance mime type, since you have the suffix in the file name already.

@tkoleary: A file can be used at multiple places, which is what the whole usage counter is about. Are you proposing a file should have multiple rows in that case or only show the first use of the file? Both sound problematic.

- The clickable number provides very little click target to get to the usage list. Can we make it '1 use' or something like that?

The heading is awkward as well.

My suggestion would be

thead: USED IN
td: 1 place

Add comment about PDX sprint.

@Gábor Hojtsy

That's right. I misread the comment. It should link to a new view.

@slashrsm

Mime type text field should have an autocomplete. Mime types are not necessarily common knowledge.

@slashrsm

Used in count does not appear to be functioning properly.

@tkoleary
Thanks for spotting that, but this is not a problem of this issue. can you please fill a different bug report for it?

+++ b/core/modules/file/config/views.view.file_usage.ymlundefined
@@ -0,0 +1,378 @@
+  file_usage:
+    display_plugin: page

Please don't let us renamed displays, ... and just use page_1 instead.

+++ b/core/modules/file/config/views.view.file_usage.ymlundefined
@@ -0,0 +1,378 @@
+module: views

+++ b/core/modules/file/config/views.view.files.ymlundefined
@@ -0,0 +1,541 @@
+module: views

Should this be better "file" instead of views?

+++ b/core/modules/file/config/views.view.files.ymlundefined
@@ -0,0 +1,541 @@
+  files:
...
+    id: files

See above.

+++ b/core/modules/file/file.moduleundefined
@@ -1661,3 +1661,40 @@ function file_library_info() {
+      'description' => user_access('access files overview')
+        ? t('Get an overview of <a href="@url">all files</a>.', array('@url' => url('admin/content/files')))
+        : t('Get an overview of all files.'),

Are there other places in core with such a dynamic description? This one here feels really wrong, because it could easily lead to infinite recursion after some refactoring.

+++ b/core/modules/file/file.moduleundefined
@@ -1661,3 +1661,40 @@ function file_library_info() {
+ * @param $choice
...
+ * @return

Please add typehinting ...

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,62 @@
+ * Definition of Drupal\file\Plugin\views\field\EntityLabel.

Needs @contains

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,62 @@
+   * Overrides \Drupal\views\Plugin\views\field\FieldPluginBase::init().
...
+  protected function defineOptions() {
...
+  public function buildOptionsForm(&$form, &$form_state) {
...
+  function render($values) {

@inheritdoc.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,62 @@
+    $options['link_to_entity'] = array('default' => FALSE, 'bool' => TRUE);

I don't think we need bool anymore in Drupal 8.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,62 @@
+    $entity = entity_load($this->getValue($values, 'type'), $this->getValue($values));

Instead of loading every entity you could also write a preRender method which collects the entities per entity type and load the in bulk.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,62 @@
+      return l($entity->label(), $uri['path'], $uri['options']);

Shouldn't we better use the views build in link functionality instead of calling l() directly?

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,62 @@
+      return check_plain($entity->label());

Should be better $this->sanitizeValue($entity->label());

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,110 @@
+ * Definition of Drupal\file\Tests\FileListingTest.

Contains ...

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,110 @@
+  protected function sumUsages($usage) {

Needs docs.

Thank you!!

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -20,19 +20,32 @@
+  private $loaded_referncers = array();

Let's use protected instead as people might want to extend the class. ... Should be also camel case, if possible.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -52,10 +68,26 @@ function render($values) {
+      $this->loaded_referencers[$type] = entity_load_multiple($type, $ids);

Instead of calling entity_load_multiple directly you could also inject the entity manager into this class and use the storage controller of them.

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -37,6 +37,15 @@ function setUp() {
+   * @param $usage
...
+   * @return

Should be @param array ... An array of file usage.

+++ b/core/modules/file/file.moduleundefined
@@ -1661,3 +1661,40 @@ function file_library_info() {
+function _views_file_status($choice = NULL) {

There should be a follow up which tries to move this into a autoloadable place.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,119 @@
+ * Contains Drupal\file\Plugin\views\field\EntityLabel.

Just another nitpick: Should be "Contains \..."

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,119 @@
+  protected $loadedReferncers = array();

Is referncers a valid english word ^^

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,119 @@
+   * Constructs entity label field class.

Let's move the constructor to the top if you are here anyway... I guess we need do document at least the entity manager.

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,118 @@
+   * @return integer

Afaik we typehint with int instead of integer.

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,118 @@
+    // Anonymous and "normal" authenticated users should not see file listing.
+    $this->drupalGet('admin/content/files');
+    $this->assertResponse(403);
+    $this->drupalLogin($this->base_user);
+    $this->drupalGet('admin/content/files');
+    $this->assertResponse(403);

This is quite intensive test coverage.

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,118 @@
+      $file = entity_load('file', $node->file[Language::LANGCODE_NOT_SPECIFIED][0]['fid']);
...
+    $orphaned_file = $nodes[1]->file[Language::LANGCODE_NOT_SPECIFIED][0]['fid'];
+    $used_file = $nodes[0]->file[Language::LANGCODE_NOT_SPECIFIED][0]['fid'];
+    $nodes[1]->file[Language::LANGCODE_NOT_SPECIFIED][0]['fid'] = $used_file;

Can't we use entity NG api now?

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,118 @@
+    $this->assertTrue(preg_match('/views-field-status priority-low\">\s*' . t('Temporary') . '/', $this->drupalGetContent()), t('Unused file marked as temporary.'));

This seems to be better implemented as $this->xpath.

Related #2022297: [META] Unified toolset for Views in core

Tried the views out again! The only quick feedback I'd have is its strange you can filter for the mime type but not the name of the file. That sounds like would be an easy addition :)

referncers
referencers

The one in the patch is spelled wrong.

Maybe I am stupid but in this spelling it just feels wrong: "Referncers"

Comment number 65 was not adressed yet.

Additional on admin/content/files/usage/1 I think there should be also some breadcrumb/local task going on.

Fixed the test to use xpath, assertFieldByXPath() is just for input fields.

If you build the view correctly, it will add the hook_menu you need.

@slashrsm

Jessebeach will know that. Maybe ping on IRC

+++ b/core/modules/file/config/views.view.files.yml
@@ -0,0 +1,579 @@
+uuid: 4b47e09e-16e0-494b-b447-7166191dbb6e

This should be removed from the export, I think.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/EntityLabel.php
@@ -0,0 +1,128 @@
+namespace Drupal\file\Plugin\views\field;
...
+class EntityLabel extends FieldPluginBase {

I see nothing file-specific in here, is there any reason this is not in views.module directly? Or entity.module for that matter?

We don't remove uuids from exports in core anymore.

Oops, sorry! I thought we did. Thanks for clearing that up.

@dawehner and why we don't remove them anymore?

Because it just makes sense to provide a UUID, so that you always can no that the view you are dealing with is the one coming from the original provided one.

#96: 2005166_95.patch queued for re-testing.

Here is a final nitpick review before setting to this to be ready! What a huge issue!

+++ b/core/modules/file/file.moduleundefined
@@ -1661,3 +1661,40 @@ function file_library_info() {
+ * @param $choice
...
+ * @return

Let's document the types.

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,117 @@
+ * Contains of Drupal\file\Tests\FileListingTest.

Should be "Contains \"

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,128 @@
+   * Constructs entity label field class.

"Constructs a EntityLabel object.".

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,128 @@
+   * @param $manager \Drupal\Core\Entity\EntityManager

the variable name should be at the end.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,128 @@
+    return new static($configuration, $plugin_id, $plugin_definition, $container->get('plugin.manager.entity'));

Let's use multiple lines for adaption later.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,128 @@
+   * @inheritdoc.
...
+   * @inheritdoc.
...
+   * @inheritdoc.
...
+   * @inheritdoc.
...
+   * @inheritdoc.

Should be "{@inheritdoc}"

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,128 @@
+  function render($values) {
...
+  function preRender(&$values) {

Afaik both should be public.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,128 @@
+    foreach ($values as $value) {
+      $loads[$this->getValue($value, 'type')][] = $this->getValue($value);
+    }
+
+    foreach ($loads as $type => $ids) {
+      $this->loadedReferencers[$type] = $this->entityManager->getStorageController($type)->load($ids);
+    }

Nice, simple and performant code.

Thank you very much! I am so glad that people took over these conversions, as this is the only chance to get them done.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

It seems strange to have a function called _views_file_status in file.module. Also, it would be nice if this could be replaced with an OO equivalent somehow ....

ok, if there is a follow up. Great. Is there?

+++ b/core/modules/file/file.moduleundefined
@@ -1660,3 +1660,40 @@ function file_library_info() {
+function file_permission() {
+  $perms = array(
+    'access files overview' => array(
+      'title' => t('Access the Files overview page'),
+      'description' => user_access('access files overview')
+        ? t('Get an overview of <a href="@url">all files</a>.', array('@url' => url('admin/content/files')))
+        : t('Get an overview of all files.'),
+    ),
+  );
+
+  return $perms;

Why we don't just return an array like on other (core) modules? It's the same but it's confusing :/

The node module needs that because it manipulates the permission but other modules like user, views, tour, toolbar, statistics, aggregator, contextual, language, locale and menu just return an array :)

It's funny to see that that taxonomy module uses $permissions instead of $perms but we should only put the permissions into a variable if we want to manipulate them.

It doesn't really matter from my point of view, so let's do whatever we have now.

#113/#114: like #115 says, it doesn't matter. There's no standard for this, and it'd be silly to have a standard for it. What matters is what is returned. Many hook_menu() implementations in D6/D7 also did something like $items = array() first, while they could just as well have done return array(…). It really doesn't matter at all.

As I said above it's just confusing... $perms, $permissions, return array().

#117: it's not confusing at all. It just requires a very tiny additional amount of code reading.

+++ b/core/modules/file/config/views.view.file_usage.ymlundefined
--- /dev/null
+++ b/core/modules/file/config/views.view.files.ymlundefined

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,133 @@
+    $uri = $entity->uri();
+    if (!empty($this->options['link_to_entity'])) {
+      $this->options['alter']['make_link'] = TRUE;
+      $this->options['alter']['path'] = $uri['path'];
+    }

Let's put $uri = $entity->uri(); inside the if as we only use $uri there...

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -0,0 +1,133 @@
+    $loads = array();
+    foreach ($values as $value) {
+      $loads[$this->getValue($value, 'type')][] = $this->getValue($value);
+    }
+
+    foreach ($loads as $type => $ids) {
+      $this->loadedReferencers[$type] = $this->entityManager->getStorageController($type)->load($ids);
+    }

As we're changing code here... let's not use a variable name $loads... and lets use loadMultiple if $ids is an array of ids...

@slashrsm I think you went a bit overboard with integrating the method call and the array lookup :) @alexpott merely asked to move the method call inside the if(), not to merge it with the array lookup :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.phpundefined
@@ -120,13 +119,13 @@ public function render($values) {
+    $entity_ids = array();

Can we find a variable which refers to entity ids per entity type, but I can't think of a proper one.

No no: $entity_id_per_type ;)

Good idea, so $entity_ids_per_type ?

Looks good, fixes all @alexpott's concerns.

Nice... views really is a great addition to core :)

Committed 8cf7830 and pushed to 8.x. Thanks!

This is great! Do we have a follow-up for a menu item (I don't want to file any duplicates)? Back in #65 we started a discussion over this, but never filed an actual issue for it. According to #94 that task is blocked on #2027043: Allow page displays to be both local tasks and menu items.

+++ b/core/modules/file/lib/Drupal/file/Tests/FileListingTest.phpundefined
@@ -0,0 +1,117 @@
+ * Contains of \Drupal\file\Tests\FileListingTest.

"Contains of" is that our new standard :P

Let's fix that in a follow-up right away because I bet we would miss that when we convert the left-over "Definition of" stuff with a regex search&replace.

diff --git a/core/modules/file/config/views.view.file_usage.yml b/core/modules/file/config/views.view.file_usage.yml
new file mode 100755
index 0000000..8b76872
...
diff --git a/core/modules/file/config/views.view.files.yml b/core/modules/file/config/views.view.files.yml
new file mode 100755
index 0000000..b0b182d

lets fix those too..should be 644

Seems legit.

Committed 6c117d6 and pushed to 8.x. Thanks!

Great, thanx! What about my question in #133 about a menu item?

@klonos: let's open a followup and postpone on #2027043: Allow page displays to be both local tasks and menu items. We cannot solve it as-is yet without that pre-requisite.

Do you mean there's a follow-up for adding a menu item, or a follow-up for #2027043? If there's a follow-up for adding the menu item, could you please post a link? Thanx.

#2047893: Add menu item(s) for the new admin/content/file view

;)

Cross-linking with a tiny followup issue: #2059699: Remove t() from test assertion messages in file.module

added the issue asked for in 109-110, 112