Views generic field handler does not work with base fields

Duh, wrong title :D

Tagging for review in today's office hours

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -345,7 +345,9 @@ protected function getFieldDefinitions() {
   +      // @fixme Yeah sounds sensible but it blocks views from rendering a field.
   +      // $this->fieldDefinitions = array_filter($definitions, array($this, 'fieldHasDisplayOptions'));
   +      $this->fieldDefinitions = $definitions;
   

   ahh what?

2. +++ b/core/modules/field/config/schema/field.views.schema.yml
   @@ -18,7 +18,7 @@ views.argument.field_list_string:
   +  label: 'Views field field handler'
   

   Views field handler or base field handler.

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -138,15 +148,21 @@ class Field extends FieldPluginBase implements CacheablePluginInterface, MultiIt
   +    // @todo On the longrun just use either 'entity field' or 'field_name' all
   +    //   over the project.
   

   Can we create an issue for that?

4. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -287,15 +273,11 @@ public function query($use_groupby = FALSE) {
   +      // @todo what we are doing here is right?
   

   So you are telling me that there is some POC in views which you don't know.

5. +++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
   @@ -0,0 +1,122 @@
   +class FieldFieldTest extends ViewUnitTestBase {
   

   Why not field handler?

6. +++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
   @@ -0,0 +1,122 @@
   \ No newline at end of file
   

   EOF missing.

7. +++ b/core/modules/views/src/Tests/ViewUnitTestBase.php
   --- /dev/null
   +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_field_field_test.yml
   

   Can you stop manually writing views? :P
   We have nice config import/export now. :)

Manual testing coming up.

Patch no longer apply so reroll.

@dawehner: french pedant superpower, à votre service ;-)

I saw #4 earlier, and was wondering about how we'd do this too.

So currently base field definitions in Entity::baseFieldDefinition() can use setDisplayOptions() to opt-in for "get rendered by a widget / formatter *in the regular entity forms / entity displays*". If they don't do so, it means they don't want to be displayed, or are displayed using dedicated legacy/custom code.

EntityDisplays are the ones in charge of actually building the formatter output when rendering an entity in a given view mode. Thus, they are the ones that implement this "ignore fields who didn't opt-in" behavior. That way, even if I hand-edit my core.entity_view_display.*.*.*/yml files and force-add fields that don't want to be rendered, they still don't get rendered.

That's fine for entity_view($entity, $view_mode) - the "official" display of an entity in one of the "official" view modes.

But since EntityDisplays are the only ones that know how to run a formatter, they are also used for "non-view mode based" rendering ("render a field using an arbitrary formatter") - typical case being Views.

This is done using throwaway runtime EntityDisplay objects for a fake '_custom' view mode (see EntityViewBuilder::getSingleFieldDisplay()). For those, ignoring base fields that opted out of "display me using a formatter on entity_view($regular_view_mode)" is not right.

The "mythical" plan for optimizing Views rendering in #1867518: Leverage entityDisplay to provide fast rendering for fields still intends to rely on those runtime EntityDisplay objects, just using them more efficiently. So IMO those are here to stay.

So I'm thinking we should formalize a bit more this notion of "EntityDisplay for an actual view mode" vs "EntityDisplay assembled at runtime, using _custom view mode", and have them behave differently :
- for "official view modes", only consider fields whose definition states they "usually" (regular entity_view()) want to be rendered
- for "non-official view modes", render all fields according to the options held in the EntityDisplay

Attached patch does :
- EntityDisplayBase::$mode defaults to '_custom' (could possibly be a class constant instead), so that a Display created without passing an explicit mode is considered a "custom" one,
- EntityViewBuilder::getSingleFieldDisplay() calls EntityDisplayBase::create() without passing 'mode',
- EntityDisplayBase::getFieldDefinitions() filters or not, depending on whether $this->mode is '_custom'.

Also - would be interesting to have @effulgentsia's thoughts about #15, since he was also involved in the design of "base fields intergration with EntityDisplays" back then.

This issue has been frenched.

Thank you for the fixes and french exposure.

What about using EntityField? This could be less confusing, than the existing solution.

Perfact

#2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is postponed on this now.

Iterating on #15 [edit: but including #18 of course]
- added some comments
- for those "arbitrary runtime EntityDisplays", we should also skip populating defaults in init(). The render will be made with the options explicitly assigned by the caller.

Side note about EntityDisplayBase::getFieldDefinitions() : #2409661: Remove duplicate check for "FieldableEntity" in EntityDisplayBase

Some nits.

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -213,39 +219,42 @@ public function toArray() {
   +      $extra_fields = \Drupal::entityManager()->getExtraFields($this->targetEntityType, $this->bundle);
   +      $extra_fields = isset($extra_fields[$context]) ? $extra_fields[$context] : array();
   

   Can we change the @var name here? IMHO it is confusing.

2. +++ b/core/modules/field/config/schema/field.views.schema.yml
   @@ -18,7 +18,7 @@ views.argument.field_list_string:
   +  label: 'Views field field handler'
   

   I thought we agreed on Views field entity field handler.

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -138,15 +150,21 @@ class Field extends FieldPluginBase implements CacheablePluginInterface, MultiIt
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, FormatterPluginManager $formatter_plugin_manager, FieldTypePluginManagerInterface $field_type_plugin_manager, LanguageManagerInterface $language_manager, RendererInterface $renderer) {
   

   Doc update needed?

4. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -172,7 +191,7 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $field_storage_config = $this->getFieldStorageDefinition();
   
   @@ -370,12 +353,8 @@ public function clickSort($order) {
   +    $field_storage = $this->getFieldStorageDefinition();
   
   @@ -383,12 +362,34 @@ public function clickSort($order) {
   +    $field_storage = $this->getFieldStorageDefinition();
   

   We should also rename the @var name.

5. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -983,4 +988,22 @@ public function getCacheContexts() {
   +   * @return \Drupal\Core\Entity\Sql\DefaultTableMapping
   

   Needs doc update.

6. +++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
   @@ -0,0 +1,121 @@
   +    $random_number = (string) 30856;
   

   LOL

7. +++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
   @@ -0,0 +1,121 @@
   +  public function testSimpleExecute() {
   ...
   +  public function testSimpleRender() {
   

   function doc missing.

8. +++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
   @@ -0,0 +1,121 @@
   +      ['id' => 5, 'field_test' => 6],
   +    ], ['id' => 'id', 'field_test' => 'field_test']
   

   nit pick can we move it to new line.

9. +++ b/core/modules/views/src/Tests/ViewUnitTestBase.php
   @@ -8,6 +8,7 @@
   +use Drupal\views\Plugin\views\field\Field;
   

   Any reason for this?

10. +++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
    @@ -0,0 +1,98 @@
    +   * @covers ::getEntityType
    ...
    +   * @covers ::getEntityType
    
    +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php
    @@ -0,0 +1,549 @@
    +  public function testConstruct() {
    ...
    +  public function testDefineOptionsWithNoOptions() {
    ...
    +  public function testDefineOptionsWithDefaultFormatter() {
    ...
    +  public function testCalculateDependenciesWithBaseField() {
    ...
    +  public function testCalculateDependenciesWithConfiguredField() {
    ...
    +  public function testAccess() {
    ...
    +  public function testClickSortWithOutConfiguredColumn($order) {
    ...
    +  public function testClickSortWithBaseField($order) {
    ...
    +  public function testClickSortWithConfiguredField($order) {
    ...
    +  public function testQueryWithGroupByForBaseField() {
    ...
    +  public function testQueryWithGroupByForConfigField() {
    ...
    +  protected function getBaseFieldStorage() {
    ...
    +  protected function getConfigFieldStorage() {
    ...
    +  public function providerSortOrders() {
    

    Are we not going to add any function desc docs for these?

11. +++ b/core/modules/views/tests/src/Unit/Plugin/HandlerTestTrait.php
    @@ -0,0 +1,62 @@
    +  protected function setupExecutableAndView() {
    ...
    +  protected function setupViewsData() {
    ...
    +  protected function setupDisplay() {
    

    function doc missing.

12. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php
    @@ -0,0 +1,549 @@
    +class FieldTest extends UnitTestCase {
    

    Class desc missing.

@jibran #23.1 : out of scope here, the patch just wraps existing code in an if().

After 3 days #11 here is the manual review.
To test after applying the patch I added $data['node_field_data']['title']['field']['id'] = 'field'; in NodeViewsData rebuild cache and went to admin/structure/views/nojs/handler/content/default/field/title
Before

We have no field formatter.
After

We have a plain text formatter.

Thank you @dawehner for fixing the review and helping me in manual testing. It is a critical bug so I don't think we need BE here.

Yay, that was quick. #2384863: Translation language base field handler should use views field handler, provide unified options will be able to immediately use this once it lands :)

A few nits.

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -23,6 +23,12 @@
   +  const CUSTOM_MODE = '_custom';
   

   Can we get an issue to throw an exception and prevent the UI from creating a mode with this id? @dawehner says this is an existing issue so a followup is fine.

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -134,19 +146,27 @@ class Field extends FieldPluginBase implements CacheablePluginInterface, MultiIt
   +    // @todo On the longrun just use either 'entity field' or 'field_name' all
   +    //   over the project.
   

   In the long run also... issue?

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -287,15 +277,10 @@ public function query($use_groupby = FALSE) {
   -      $rkey = $this->definition['is revision'] ? EntityStorageInterface::FIELD_LOAD_REVISION : EntityStorageInterface::FIELD_LOAD_CURRENT;
   +      $rkey = !empty($this->definition['is revision']) ? EntityStorageInterface::FIELD_LOAD_REVISION : EntityStorageInterface::FIELD_LOAD_CURRENT;
   

   $rkey is no longer used - we can remove this line.

4. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -287,15 +277,10 @@ public function query($use_groupby = FALSE) {
   -        $name = $column;
   -        if (isset($field_definition['storage_details']['sql'][$rkey][$this->table][$column])) {
   -          $name = $field_definition['storage_details']['sql'][$rkey][$this->table][$column];
   -        }
   -
   -        $fields[$column] = $name;
   

   yay! I've been wanting to remove views hard-coding of sql storage for ages.

5. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -383,13 +364,35 @@ public function clickSort($order) {
   +    // @todo unify the two options here.
   

   @todo should have the same issue linked as the @todo above.

6. +++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
   @@ -0,0 +1,98 @@
   +
   +class TestHandler extends HandlerBase {
   +
   +}
   

   At least a small PHPDoc for the class would be nice

7. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php
   @@ -0,0 +1,569 @@
   +      // @todo replace the second anything() with FALSE.
   

   A @todo in an added test feels odd. I think this needs more explanation.

#28.1 : opened #2410727: Prevent UI from editing / saving EntityDisplays for '_custom' view mode

Updated the patch for those suggestions except #28.7. Opened #2410779: Entity views data uses 'entity field', naming incompatible with rest of core using 'field_name' for the entity field / field_name todo. The reason of the @todo spotted in #28.7 is not clear to me. That refers to the $return_as_object = FALSE argument on fieldAccess().

Only doc fixes so back to RTBC,

As I wrote #28.7 is not fixed yet.

Seems like that second anything() is totally replaceable with FALSE, so I think this was intended as a pre-commit @todo... Works locally.

Thanks @Gábor Hojtsy for the fixes.

Some things I missed.

1. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -92,7 +92,6 @@ public static function createFromFieldStorageDefinition(FieldStorageDefinitionIn
   -      ->setRequired($definition->isRequired())
   

   How come?

2. +++ b/core/modules/field/config/schema/field.views.schema.yml
   @@ -27,8 +27,9 @@ views.field.field:
   +      type: field.formatter.settings.[%parent.type]
   +      type: sequence
   

   two types?

A minor thing if this is rerolled:

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -213,39 +222,42 @@ public function toArray() {
   +      $extra_fields = \Drupal::entityManager()->getExtraFields($this->targetEntityType, $this->bundle);
   

   We can use $this->entityManager() here.

2. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -92,7 +92,6 @@ public static function createFromFieldStorageDefinition(FieldStorageDefinitionIn
   -      ->setRequired($definition->isRequired())
   

   Sorry if this was already explained, but why is it needed?

Edit: sorry, I missed Alex's post.

@alexpott: fixed schema.

@plach: fixed the non-use of injected entity manager but this method had several global entity manager uses, so those need conversion also then.

@alexpott/@plach: FieldStorageDefinitionInterface does not have an isRequired() method on it, so it should not be expected to work pre-path already. Let's try to add it back and see if it falls down in flames or not.

And as expected: PHP Fatal error: Call to undefined method Mock_FieldStorageDefinitionInterface_eab0a1f2::isRequired() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Field/BaseFieldDefinition.php on line 95. Looking at the interfaces, isRequired() is on the FieldDefinitionInterface level. Why do we expect to be able to call it on a FieldStorageDefinitionInterface?

BaseFieldDefinition::createFromFieldStorageDefinition() calling $storage_definition->isRequired() :
Right, that seems wrong. Wondering why it's only revealed by this patch though ?

@yched: the patch changes Drupal\views\Plugin\views\field::getFieldDefinition() which is the only place that invokes BaseFieldDefinition::createFromFieldStorageDefinition() to invoke it with a FieldStorageDefinition (as opposed to before this patch it was invoking it with a FieldStorageConfig which does have an isRequired method). FieldStorageConfigInterface does not have isRequired either, so although FieldStorageConfig says @inheritdoc on its isRequired implementation, none of its interfaces mandates to implement that method, it is just there in itself. FieldStorageConfig has a hardwired FALSE return for isRequired() so not taking that value and not passing into setRequired() in BaseFieldDefinition::createFromFieldStorageDefinition() does not practically change anything about the behavior.

Not sure if FieldStorageConfig::isRequired() is expected to be there elsewhere despite it not being on the interface. Opened #2411323: FieldStorageConfig::isRequired should not exist so we are not experimenting with that here.

Should be RTBC as per above, the changes since then are minor.

Great progress here! Seems like an elegant, fitting solution.

@Gábor: Makes sense, thanks. Let's followup in #2411323: FieldStorageConfig::isRequired should not exist

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