File views handlers need to be replaced with entity-aware formatters

This sounds very interesting, and a lot of work. Is #2456713: Custom taxonomy field views handler needs to be replaced with generic Field API handler a good example of what needs to be done for File to get it into compliance?

I'll give it a try.

Let's see, how much fails.

I think we need @yched input here.

We also need tests for all the new formatters.

@jibran
Sure, no question.

Here is some work on config schema first.

Fixing just the test for now.

1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/BaseFieldFileFormatterBase.php
   @@ -0,0 +1,84 @@
   +abstract class BaseFieldFileFormatterBase extends FormatterBase {
   

   The name of this class is a bit unfortunate, I don't think it's tied only to base fields, is it?

2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/DefaultFileFormatter.php
   @@ -0,0 +1,50 @@
   +class DefaultFileFormatter extends BaseFieldFileFormatterBase {
   
   +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileExtensionFormatter.php
   @@ -0,0 +1,83 @@
   +class FileExtensionFormatter extends BaseFieldFileFormatterBase {
   
   +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileSize.php
   @@ -0,0 +1,47 @@
   +class FileSize extends FormatterBase {
   
   +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileUriFormatter.php
   @@ -0,0 +1,59 @@
   +class FileUriFormatter extends BaseFieldFileFormatterBase {
   
   +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FilemimeFormatter.php
   @@ -0,0 +1,75 @@
   +class FilemimeFormatter extends BaseFieldFileFormatterBase {
   

   Wouldn't it be easier to just provide something like "FileInformationFormatter" with settings to display all of these things?

3. +++ b/core/modules/file/src/Tests/Formatter/DefaultFileFormatterTest.php
   @@ -0,0 +1,18 @@
   +class DefaultFileFormatterTest extends KernelTestBase {
   +}
   

   I think the test are failing because of this test class with no test*() methods.

Thank you for your review!!

Wouldn't it be easier to just provide something like "FileInformationFormatter" with settings to display all of these things?

Did you had a look at the code and see what they actually provide, its so dramatically different.

I think the test are failing because of this test class with no test*() methods.

Yeah right, I was not able to motivate to write some, if we aren't even sure whether we want the formatter structure like that.

Alright, we have tests now.

Thank you for your intensive review!

c) core/modules/file/src/Plugin/Field/FieldFormatter/BaseFieldFileFormatterBase.php -- class has no class doc block.

Made it to exactly 80 chars.

Can we fix the field title/description? These don't make much sense to me. How about:

Sure ... to be clear, I just copied them over from where, where especially the description might make more sense in the first place.

Link this field to the file download URL

That is absolutely an improvement.

Um... is this right? $items is an array of field items, right? So ... how is there only one entity URL? You have a multi-valued field and you're making the same link for all of them??!? That cannot possibly be right?

... Field formatters are always written against FieldItemList ... as this is what $entity->bar always contains, no matter whether its a base field or its a configured field, with actually multiple entries.
Please have a look at any other formatter in core. ... $items is always coming from one $entity.

i) Code in FileExtensionFormatter... kind of messy and unclear, why not use pathinfo() PHP function instead of custom regular expressions? Also the code comment "If there is an extension." is not a sentence.

Well, its all existing code ... too bad

"Formatter for the filesize field on the File entity."

Alright

k) Shouldn't all these formatters do a check in isApplicable() to make sure the fields are on a File entity?

Well, I guess we have to ... Its just a bit sad that we can't make the formatters easily reusable.

What does that description mean? Would anyone not a Drupal developer have any idea what the "internal filestream address" is?

Well, I just removed it, we seem to not want to explain users details, too bad.

That is kind of long, maybe split into title/description?

Did that, thank you.

Hm. So if you set up to use the mime image, you are deriving that image from the entire File entity, not the mime type field? That's interesting...

Well, it should be derived though from the filemime at the end ...

q) core/modules/file/src/Tests/Formatter/FileEntityFormatterTest.php - most methods lack docblocks

Right, because I think adding docblocks here would not add value, but rather discourage people from finding good test method names ...

Whew!

Seriously, use dreditor, it makes life so much easier ...

So we do have test coverage for that view, yeah!

Per #2393339-57: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, tagging for critical office hours, but if there's a reason to not have this particular child issue in that list, please untag it.

This is close too, just some minor cleanup and some questions around optimisations

1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/DefaultFileFormatter.php
   @@ -0,0 +1,58 @@
   +  public function settingsForm(array $form, FormStateInterface $form_state) {
   

   This method can go?

2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileExtensionFormatter.php
   @@ -0,0 +1,80 @@
   +    return $field_definition->getTargetEntityTypeId() === 'file' && $field_definition->getName() === 'filename';
   

   Is it worth moving the first half of this to the parent method? Seems to be common to all of the child classes. Then these lines would be:

   return parent::isApplicable($field_definition) && {the rest of the hunk}
   

3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileSize.php
   @@ -0,0 +1,47 @@
   + * Formatter that shows the extension on the filename field for a File entity.
   

   c/p error - should say something about file size

4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FilemimeFormatter.php
   @@ -0,0 +1,76 @@
   + * Formatter to render filemime as image.
   

   Seems that rendering image is optional? Perhaps Formatter to render the file mime type, with an optional icon?

5. +++ b/core/modules/file/src/Tests/Formatter/FileEntityFormatterTest.php
   @@ -0,0 +1,140 @@
   +    $file->save();
   

   Any reason for the two file->save() calls (here and in all cases)

6. +++ b/core/modules/file/src/Tests/Formatter/FileEntityFormatterTest.php
   @@ -0,0 +1,140 @@
   +  public function testFormatterFileLink() {
   ...
   +  public function testFormatterFileExtension() {
   ...
   +  public function testFormatterFileMime() {
   ...
   +  public function testFormatterFileSize() {
   

   Missing docblocks?

NW cuz of #19

Is it worth moving the first half of this to the parent method? Seems to be common to all of the child classes. Then these lines would be:

Well, if you think its really easier to read :)

Any reason for the two file->save() calls (here and in all cases)

impatience

Missing docblocks?

Meh

Manual testing - ended up with an Exception - see screenshot.

Also noticed that the boolean formatter doesn't seem to have #states support, although could be pre-existing issue there.

Attached is my test view from which the exception was thrown - created the view, added an article with a file attached.

Thank you for your manual test! I think we should

Manual testing - ended up with an Exception - see screenshot.

Long story short.

Fatal error, due to passing "0" as $settings. This is caused by the code in core/lib/Drupal/Core/Field/FormatterPluginManager.php:161
doing basically array_intersect_key($settings, NULL) + NULL === NULL. This is caused by a missing return value in \Drupal\file\Plugin\Field\FieldFormatter\FileUriFormatter::defaultSettings

Another problem was that the wrong field type was supported.

Let's expand the test coverage.

Attached is my test view from which the exception was thrown - created the view, added an article with a file attached.

Yeah, that is not a problem of that issue, but I was sure #1985406: #states not supported for elements in formatter settings being displayed on Views field handler form had fixed that.

Also tests has to be written in the right ways.

Added the access test coverage

Thanks

Just fixed a nitpick.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed d36e3de and pushed to 8.0.x. Thanks!

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 7f90ee6 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/file/src/Plugin/Field/FieldFormatter/DefaultFileFormatter.php b/core/modules/file/src/Plugin/Field/FieldFormatter/DefaultFileFormatter.php
index fb51e9f..18375aa 100644
--- a/core/modules/file/src/Plugin/Field/FieldFormatter/DefaultFileFormatter.php
+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/DefaultFileFormatter.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\file\Plugin\Field\FieldFormatter;
 
-use Drupal\Core\Field\FieldDefinitionInterface;
 use Drupal\Core\Field\FieldItemInterface;
 use Drupal\Core\Form\FormStateInterface;
 
diff --git a/core/modules/file/src/Tests/Views/FileViewsFieldAccessTest.php b/core/modules/file/src/Tests/Views/FileViewsFieldAccessTest.php
index e7593fc..74baac3 100644
--- a/core/modules/file/src/Tests/Views/FileViewsFieldAccessTest.php
+++ b/core/modules/file/src/Tests/Views/FileViewsFieldAccessTest.php
@@ -9,8 +9,6 @@
 
 use Drupal\file\Entity\File;
 use Drupal\language\Entity\ConfigurableLanguage;
-use Drupal\node\Entity\Node;
-use Drupal\node\Entity\NodeType;
 use Drupal\user\Entity\User;
 use Drupal\views\Tests\Handler\FieldFieldAccessTestBase;

Fixed some unused uses on commit.

Opened a small follow-up to remove some forgotten entries in the file module's config schema: #2479607: Remove obsolete schema entries from file.views.schema.yml