Provide test coverage for access checking of all views fields.

Let's make this a child issue of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality

Can we expand this test to one of the entities from #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality that still has a critical follow-up, to verify that the test would catch the problem?

Since we're not going to add test coverage on issue like #2456691: User email field need to use Field-Entity-aware formatters in Views this needs to be critical.

@dawehner -- I got dreditor, let's see how it went... Anyway this patch looks great! I just had a couple of ideas about API docs. Surprise, surprise. ;)

Also we need to:
a) Make sure all the base fields are in there for all the entities.
b) Make sure that on the "convert this to use a proper handler" issues, we uncomment the test lines, or else have a follow-up @todo with an issue to uncomment them all and make sure the tests still pass.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/BasicStringFormatter.php
   @@ -19,7 +19,8 @@
   + *     "email",
   + *     "uuid",
   

   Should this be part of this patch? Seems like it belongs somewhere else?

2. +++ b/core/modules/aggregator/src/Tests/Views/AggregatorFeedViewsFieldAccessTest.php
   @@ -0,0 +1,50 @@
   + * Tests aggregator_item views field access.
   

   Maybe these first-line class descriptions should be something more like:

   Tests base field access in Views for the foo entity. ??

3. +++ b/core/modules/views/src/Tests/Handler/FieldFieldAccessTestBase.php
   @@ -0,0 +1,123 @@
   +   * Callers to this method already have to create entities.
   

   Can we change this to something like:

   To use this method, set up an entity of type $entity_type_id, with field $field_name. Create an entity instance that contains content $field_content in that field. This method will check that a user with permission can see the content in a view, and a user without access permission on that field cannot.

Oh, and didn't we have test coverage for access on a lot of these fields added when you did that "Convert the standard plugins to use 'field'" issue? Maybe we can remove that in favor of this test, which is clearer and comprehensive?

Looking good!

So I started looking through all the entities, from here:
https://api.drupal.org/api/drupal/8/search/basefielddefinitions
It doesn't look like this patch includes all of the base fields on all of the entities. Should it? For instance, I started at the top. The aggregator Feed entity has:
fid
uuid
title
langcode
url
refresh
checked
...

The patch has only title (commented out, as that field hasn't been converted yet), url, langcode.

Would it make sense in this test to use some sort of discovery method on the entity type to find out what the base fields are, and test all of them systematically? It seems like we want to aim for 100% test coverage...

OK, this is looking good, aside from not covering all the fields, which we cannot do now since not all of them are fixed yet.

So, should we:

a) Wait until all the other critical children on the parent are done, and make this patch cover everything then?

b) Commit it now with a follow-up to have this cover everything when the other issues are done? [add lots of @todo statements to this patch, one per test class, and a new issue they can all point to]

c) Something else?

I agree. So I filed a followup; patch just needs @todo added to refer to it. [so, needs work]
#2464635: Follow-up: Enable additional tests for base entity field access checking in Views

Oops.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -31,6 +31,7 @@
   + *   default_formatter = "entity_reference_label",
   

   Is this from a different issue? I guess it's OK to have here if it's needed.

2. +++ b/core/modules/aggregator/src/Tests/Views/AggregatorFeedViewsFieldAccessTest.php
   @@ -0,0 +1,52 @@
   +   * Check access for aggregator_feed fields.
   

   Nitpick: checks not check

3. +++ b/core/modules/aggregator/src/Tests/Views/AggregatorItemViewsFieldAccessTest.php
   @@ -0,0 +1,60 @@
   +   * Check access for aggregator_item fields.
   

   Same here. Etc.

4. +++ b/core/modules/comment/src/Tests/Views/CommentViewsFieldAccessTest.php
   @@ -0,0 +1,81 @@
   +    // @todo Expand the test coverage in https://www.drupal.org/node/2462589
   

   I think these are the wrong link? should be https://www.drupal.org/node/2464635 ?

+++ b/core/modules/node/src/Tests/Views/NodeViewsFieldAccessTest.php
@@ -0,0 +1,77 @@
+    // @todo Don't we want to display Published / Unpublished by default?
+    $this->assertFieldAccess('node', 'status', 'On');
+    $this->assertFieldAccess('node', 'promote', 'On');
+    $this->assertFieldAccess('node', 'sticky', 'Off');

is this still intended to be done in this issue, or do we need a follow-up to reference?

+++ b/core/modules/views/src/Tests/Handler/FieldFieldAccessTestBase.php
@@ -0,0 +1,141 @@
+  protected function assertFieldAccess($entity_type_id, $field_name, $field_content) {

This method is slick, love it

So given the goal is to get this in and then uncomment lines in the follow up after the conversions are done, I think this is ready.

Boldly setting to RTBC.

+1 for RTBC. And +1 for a follow-up (not at all related to this patch) that sets the default for Status field to Published/Unpublished. Actually though we should probably file that issue and either put the issue link in the @todo or take it out since that's not where the code will go. And...

+    // @todo Don't we want to display Published / Unpublished by default?
+    $this->assertFieldAccess('node', 'status', 'On');
+    $this->assertFieldAccess('node', 'promote', 'On');
+    $this->assertFieldAccess('node', 'sticky', 'Off');

Presumably Status should say "Published"/"Unpublished", promote should say "Promoted"/"Not promoted", and sticky should say "Sticky"/Not sticky. We should be able to do all of them in one issue.

Needs work to fix/remove that @todo and file the new issue.

Excellent. This looks like a really great start.

Committed and pushed to 8.0.x. Thanks!