Problem/Motivation

Views uses some metadata to describe the available tables, called views_data.
For entities it is quite an annoying task to write them by hand, because a) there are a lot of fields on entities b) getting it right is hard, as there is multilingual
as well as complex entity database stuctures involved.

Proposed resolution

Generate the views data based upon existing entity metadata.

On addition to that, let the file and node views integration be partly done by the new generic implementation
in order to have a translatable and non-translatable example in core.

Remaining tasks

User interface changes

API changes

Original report by @dawehner

CommentFileSizeAuthor
#128 interdiff.txt3.87 KBdawehner
#128 views_data-1740492-128.patch73.71 KBdawehner
#125 interdiff.txt14.86 KBdawehner
#125 views_data-1740492-125.patch73.33 KBdawehner
#122 interdiff.txt9.79 KBdawehner
#122 views_data-1740492-122.patch74.63 KBdawehner
#120 interdiff.txt478 bytesdawehner
#120 views_data-1740492-120.patch83.83 KBdawehner
#117 interdiff.txt853 bytesdawehner
#117 views_data-1740492-117.patch83.36 KBdawehner
#115 views_data-1740492-115.patch82.53 KBdawehner
#115 interdiff.txt2.15 KBdawehner
#112 interdiff.txt733 bytesdawehner
#112 views_data-1740492-112.patch80.38 KBdawehner
#111 views_data-1740492-111.patch79.67 KBdawehner
#111 interdiff.txt4.48 KBdawehner
#107 interdiff.txt7.02 KBdawehner
#107 views_data-1740492-107.patch75.17 KBdawehner
#101 interdiff.txt4 KBdawehner
#101 1740492-views_data-101.patch73.79 KBdawehner
#98 views_data-1740492-97.patch73.7 KBdawehner
#98 interdiff.txt25.11 KBdawehner
#92 interdiff.txt20.9 KBdawehner
#92 1740492-views_data-91.patch71.09 KBdawehner
#88 1740492-views_data-88.patch64.45 KBdawehner
#88 interdiff.txt4.16 KBdawehner
#82 interdiff.txt35.18 KBdawehner
#82 1740492-views_data-82.patch63.7 KBdawehner
#73 1740492-views_data-73.patch44.76 KBdawehner
#73 interdiff.txt2.43 KBdawehner
#71 interdiff.txt28.69 KBdawehner
#71 1740492-views_data-71.patch45.47 KBdawehner
#68 1740492-68.patch38.3 KBdawehner
#53 1740492.patch33.71 KBdawehner
#45 interdiff.txt7.67 KBdawehner
#42 vdc-1740492_42_interdiff.txt10.26 KBdasjo
#42 vdc-1740492_42.patch66.12 KBdasjo
#39 vdc-1740492_interdiff_39.txt34.45 KBdasjo
#39 vdc-1740492_39.patch63.81 KBdasjo
#37 interdiff-1740492.txt32.8 KBdawehner
#37 vdc-1740492.patch30.61 KBdawehner
#36 1740492-36.patch24.65 KBdamiankloip
#36 interdiff-1740492-36.txt11.37 KBdamiankloip
#35 1740492-35.patch22.59 KBdamiankloip
#35 interdiff-1740492-35.txt12.79 KBdamiankloip
#34 1740492-34.patch22.27 KBdamiankloip
#29 vdc-1740492-29.patch22.46 KBdawehner
#28 drupal-1740492-28.patch20.12 KBdawehner
#27 drupal-1740492-27.patch18.92 KBdawehner
#22 drupal-1740492-22.patch16.25 KBdawehner
#19 drupal-1740492-19.patch15 KBdawehner
#18 drupal-1740492-18.patch8.18 KBdawehner
#5 views-1740492-5.patch12.86 KBxjm
#5 interdiff-1740492-5.txt6.17 KBxjm
#1 1740492-entity-view-controller.patch12.72 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
12.72 KB

Here is the code which could work, but well core property api doesn't have a definition for node yet.

Status: Needs review » Needs work

The last submitted patch, 1740492-entity-view-controller.patch, failed testing.

tim.plunkett’s picture

Issue tags: +VDC
xjm’s picture

Assigned: Unassigned » xjm

I did a quick 15-minute skim of the patch.

  1. +++ b/lib/Drupal/views/Plugin/views/entity_controller/DefaultEntityController.phpundefined
    @@ -0,0 +1,15 @@
    +/**
    + * @Plugin(
    + *   id = "default",
    + */
    +class DefaultEntityController extends EntityControllerPluginBase {
    

    Needs a one-line summary at the top of the docblock.

  2. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +use Drupal\Component\Plugin\PluginBase;
    +
    +
    +/**
    

    Extra blank line.

  3. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    + * Generic entity views controller which utilities the entity property api.
    

    The class docblock should start with a third-person verb. API should also be all caps.

  4. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +class EntityControllerPluginBase extends PluginBase {
    +  /**
    +   * @todo
    +   */
    +  protected $entity_type;
    

    There should be a blank line between the start of the class and the first docblock.

  5. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +  public function __construct(array $configuration, $plugin_id, $entity_type) {
    

    Needs a docblock.

  6. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +  /**
    +   * Defines the result for hook_views_data().
    +   */
    +  public function viewsData() {
    

    Needs return value documentation.

  7. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +  /**
    +   * Try to come up with some views fields with the help of the schema and
    +   * the entity property information.
    +   */
    ...
    +  /**
    +   * Comes up with views information based on the given schema and property
    +   * info.
    +   */
    

    The function summary should be one line. We also need parameter and return value docs.

  8. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +      // We name the field of the first reverse-relationship just with the
    +      // base table to be backward compatible, for subsequents relationships we
    +      // append the views field name in order to get a unique name.
    

    Let's break this up into two sentences.

  9. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +      $name = !isset($this->relationships[$info['base table']][$this->entity_info['base table']]) ? $this->entity_info['base table'] : $this->entity_info['base table'] . '_' . $views_field_name;
    

    This ternary is a bit hard to scan; let's break it up.

  10. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +      $return['relationship'] = array(
    +        'label' => drupal_ucfirst($info['label']),
    

    Let's add an inline comment describing this hunk.

  11. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +      // Add in direct field/filters/sorts for the id itself too.
    

    ID should be all caps.

  12. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +      // Append the views-field-name to the title if it is different to the
    +      // property name.
    

    "...different from the property name."

  13. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +    switch ($type) {
    

    Is there a cleaner way to do this than a big switch?

  14. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +    // @todo: This class_exists is needed until views 3.2.
    

    Is this still relevant? Can we clarify it a little? What/why?

  15. +++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
    @@ -0,0 +1,313 @@
    +    return 'views_handler_relationship';
    +  }
    +
    +
    +
    +}
    

    We just need one blank line before the end of the class. :)

  16. +++ b/lib/Views/field/Plugin/views/field/Field.phpundefined
    @@ -381,7 +381,7 @@ class Field extends FieldPluginBase {
    -
    +''
    

    Oops. :)

I'll reroll touching some of this up.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
6.17 KB
12.86 KB

I didn't spend too much time on this; just added some @todo stubs where there are docs to fill in. Also not addressed:

+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
@@ -0,0 +1,313 @@
+    // @todo: This class_exists is needed until views 3.2.

Is this still relevant? Can we clarify it a little? What/why?

+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined
@@ -0,0 +1,313 @@
+    switch ($type) {

Is there a cleaner way to do this than a big switch?

xjm’s picture

Issue tags: +Needs tests
Sylvain Lecoy’s picture

Is this issue plan to make the view object an entity?

Subscribing

xjm’s picture

@Sylvain Lecoy: The goal of this issue is to provide unified entity handling for views of entity data based on the new D8 entity system; it's not related to the View objects themselves. See: http://drupal.org/node/1400186 (Rearchitecture related to the view object itself is happening separately.)

bojanz’s picture

Assigned: Unassigned » bojanz
aspilicious’s picture

The bases class should be abstract.
And does "'handler' => 'views_handler_field_numeric'," works?
Don't we need to use the plugin id?

dawehner’s picture

@aspilicious
This is just a start for the patch ...

damiankloip’s picture

Title: Implement a entity views controller » Implement a entity views data controller

Is this title better?

damiankloip’s picture

Title: Implement a entity views data controller » Implement an entity views data controller

Also, for people wondering, #1741154: Replace ctools_include('export') with Configurable Thingies is the meta issue for the view object becoming a configurable.

tim.plunkett’s picture

#5: views-1740492-5.patch queued for re-testing.

dawehner’s picture

@bojanz

This issue is probably a perfect task to talk and work together during badcamp and other badcamp-related places like in switzerland (not sure whether you attend any of those). What are your time-plans regarding this feature.

There might be problems which better should be solved somewhere in the entity storage level
so the earlier we identify potential issues the better.

dawehner’s picture

Status: Needs review » Active

Lets be honest.

xjm’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
dawehner’s picture

dawehner’s picture

Assigned: bojanz » Damien Tournoud
FileSize
15 KB

Worked on getting automatic views integration with tests for revisions.

dawehner’s picture

Assigned: Damien Tournoud » Unassigned

sorry

Berdir’s picture

This looks very interesting. #1833334: EntityNG: integrate a dynamic property data table handling is adding different test entity classes for all combinations of revision/properties tables. Sounds like this could be used here as well as soon as it is in.

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewsController.phpundefined
@@ -0,0 +1,168 @@
+  function __construct($entity_type) {
+    $this->entityType = $entity_type;
+    $this->entityInfo = entity_get_info($entity_type);
+    $this->storageController = entity_get_controller($entity_type);

To make it easier to convert to DIC later, I suggest we pass in the storage controller as an argument.

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewsController.phpundefined
@@ -0,0 +1,168 @@
+      // @todo Is translating the right way to handle label/description?
+      $views_field['title'] = t($typed_data['label']);
+      $views_field['help'] = t($typed_data['description']);

Not necessary, this is already translated, unlike the 7.x version that was based on hook_schema().

dawehner’s picture

FileSize
16.25 KB

That's a good idea, thank you!

Berdir’s picture

+++ b/core/modules/entity/entity.views.incundefined
--- /dev/null
+++ b/core/modules/entity/lib/Drupal/entity/EntityViewsController.phpundefined

Wondering where's the correct place for this. Given that the views_entity_get_views_controller() function lives in views.module, this might belong there as well.

This is views integrating with a core component, not the entity.module integrating it's tables with views.

For testing, it would be interesting to compare the generated views data with manually created views data definitions to check what we're missing here, e.g. for the node and users table. That will of course have to wait until these are converted to NG and we currently know which parts are missing anyway. But it might make sense to do a follow-up to convert existing types to this after the NG conversion.

dawehner’s picture

Yeah i have been struggling with that and moved stuff around from system to entity to views and back. One reason why it maybe should be part of entity module is that all the other core entity controllers also can be found in that directory so it might confuse people to have that in another place, but i really don't care :)

It's indeed a good idea, so some general things which are missing (but i would sort of introduce maybe with that or with another issue are things like: a generic link/uri field, maybe also label and in general every kind of single operation with a link.

Berdir’s picture

The other controllers are still in Drupal\Core\Entity, where this one definitely can't be as Core stuff can't depend on an optional module (we have quite a few user references in there, but that's a required module).

Doing additional features in follow-up issues sounds fine to me. What do we want to define as goal of this issue then? Basic support for the three tables and the existing properties? According to the test, data table support is missing, what exactly is blocking you there? I guess we need a way to identify which properties are in there (which is the same as they being translatable, right? Which is a property that I also need for TMGMT)

dawehner’s picture

Oh good point, so yeah the best place is probably views itself.

Regarding the goal of the issue: I think it should be a generic integration which works for every entity using entityNG. I don't think it makes sense to convert the existing implementations already to that controller, as it would make it really hard to review.

From my understanding #1833334: EntityNG: integrate a dynamic property data table handling blocks the data integration, but it's already long ago when I wrote that todo :)

One general problem is the question how to map typed data to handlers, currently it is hardcoded, but it feels like that we could put that information either on the typed data itself, or name the plugin_id's exactly how the typed data.

dawehner’s picture

FileSize
18.92 KB

Just some rerolling, no actual progress.

dawehner’s picture

FileSize
20.12 KB

It's a bit odd as you have different properties in the different tables, well.

dawehner’s picture

FileSize
22.46 KB

Just started to continue, but got blocked on #2024601: Provide a database schema service

bojanz’s picture

Priority: Normal » Major

I would call this major at least.
We want every entity type in contrib land to automatically have views integration, making them support
data and revision tables manually is going to lead to a buggy world of hurt.

plach’s picture

Yep, totally agreed. We've been talking about this in Dublin and it's not going to be easy but we need to come up with something.

Xano’s picture

Seeing as this would be an API addition, rather than a change, are we still going to try and get this in?

damiankloip’s picture

Yes, I think this is still feasible. I was going to have a look at this soon, it seems everyone (myself included) just doesn't have much time at the moment :/

damiankloip’s picture

FileSize
22.27 KB

Here's a reroll. Looking at this now.

damiankloip’s picture

FileSize
12.79 KB
22.59 KB

ok, this updates what is currently broken. Still a work in progress.

damiankloip’s picture

FileSize
11.37 KB
24.65 KB

Some more progress, I think we need to do something about the schema api, and making it a service.

dawehner’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +PHPUnit
FileSize
30.61 KB
32.8 KB

Continued the existing work and rewrote the tests using unit tests

Additional added some @todos in the existing code

The last submitted patch, 37: vdc-1740492.patch, failed testing.

dasjo’s picture

i have added some minor fixes to get the controller working and started converting the views data integration for the file entity to the new entity views data controller.

up next:
- find ways to improve the generic viewsData() handling of EntityViewsController & improve mapTypedDataToHandlerId()

The last submitted patch, 39: vdc-1740492_39.patch, failed testing.

fago’s picture

Created #2144631: Add a revisionable key to field definitions which would be a help to figure out the right table for a given base field.

dasjo’s picture

thanks to some great help from fago, i worked on restructuring the patch and bring in some more type data info magic.

as amateescu stated on irc, we might want to build this upon #2144327: Make all field types provide a schema()

The last submitted patch, 42: vdc-1740492_42.patch, failed testing.

dawehner’s picture

Please ensure to keep the unit tests green while developing stuff. They are written that early for a good reason.

dawehner’s picture

FileSize
7.67 KB

This tries to actually make something useful and fix stuff.

dawehner’s picture

Status: Needs review » Postponed
dasjo’s picture

yeah - @ #44 sorry that i broke the tests dawehner!

i only focused on manual testing during yesterday's sprints and couldn't finish everything due to time constraints.

still, i also included a fix to the tests by adding the missing getInfo() to EntityViewsControllerTest :)

Berdir’s picture

Status: Postponed » Needs work

I think we can unblock this, the new table mapping stuff should help views to understand which field exists in which table? If we can generate the schema, we should also be able to generate the views data?

plach’s picture

Issue tags: +beta target

I agree, and we should try to get this in before beta although that's not strictly required I guess.

Berdir’s picture

I don't see how we can release with a static and hardcoded views integration when we claim to support different table layouts, for example for node. So IMHO, this just became a lot more important.

plach’s picture

Priority: Major » Critical

I was just meaning we might be able to implement this without API breaking changes (for some definition of API™), but I agree we cannot release D8 without this.

dawehner’s picture

Assigned: Unassigned » dawehner

Working on it.

dawehner’s picture

FileSize
33.71 KB

Consider this as a git stash

andyceo’s picture

Status: Needs work » Needs review

The last submitted patch, 18: drupal-1740492-18.patch, failed testing.

The last submitted patch, 19: drupal-1740492-19.patch, failed testing.

The last submitted patch, 22: drupal-1740492-22.patch, failed testing.

The last submitted patch, 27: drupal-1740492-27.patch, failed testing.

The last submitted patch, 28: drupal-1740492-28.patch, failed testing.

The last submitted patch, 29: vdc-1740492-29.patch, failed testing.

The last submitted patch, 34: 1740492-34.patch, failed testing.

The last submitted patch, 35: 1740492-35.patch, failed testing.

The last submitted patch, 36: 1740492-36.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 53: 1740492.patch, failed testing.

dawehner’s picture

As talked with fago and various other persons before. Let's do the first small step: move the views data provider by entities into a views data controller but don't implement anything
generic yet. #2319671: ViewsDataController: Step1: Move entity views data into controllers

jhodgdon’s picture

It seems like one way to go about this would be just to provide a base class for the Views data controller, which could probably return most of the information, and then each individual one could call parent::getData() or whatever, then add more to it. But ... they're all kind of unique... may not be a big win?

dawehner’s picture

It seems like one way to go about this would be just to provide a base class for the Views data controller, which could probably return most of the information, and then each individual one could call parent::getData() or whatever, then add more to it. But ... they're all kind of unique... may not be a big win?

The idea is that we do provide the majority of views data using all the various entity metadata, like basefields and storage information. This makes it much easier for the developer, as they just have to implement special handlers like links to edit/delete.

On top of that I think core will need specific entity handlers all over the place thought those could be way smaller in the future.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.3 KB

Opened a kind of related issue: [#2281185]

Status: Needs review » Needs work

The last submitted patch, 68: 1740492-68.patch, failed testing.

Berdir’s picture

First look at the patch. Looks interesting. Feedback/Ideas below.

I think you have the wrong NID in #68.

There also seems to be an accidental rename in the first part of the patch that might be the reason for the failed installation?

  1. +++ b/core/modules/file/src/FileViewsData.php
    @@ -6,32 +6,29 @@
    -      'field' => 'fid',
    -      'title' => t('File'),
    +      // @TODO There is no corresponding information in entity metadata.
           'help' => t("Files maintained by Drupal and various modules."),
           'defaults' => array(
             'field' => 'filename'
           ),
    

    I guess we could introduce a description key on entity types?

    default could be the entity label key?

  2. +++ b/core/modules/file/src/FileViewsData.php
    @@ -66,56 +57,23 @@ public function getViewsData() {
     
         // Describes filename field in file_managed table.
         $data['file_managed']['filename'] = array(
    -      'title' => t('Name'),
    -      'help' => t('The name of the file.'),
           'field' => array(
             'id' => 'file',
    

    I assume here and elsewhere, it should be += array, otherwise you'd replace whatever is already there?

  3. +++ b/core/modules/file/src/FileViewsData.php
    @@ -495,8 +406,7 @@ public function getViewsData() {
     
    -    return $data;
    +    return NestedArray::mergeDeep($parent_views_data, $data);
    

    Ah, that's how you do it, would it be easier to use += array() ?

  4. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,301 @@
    +    $field_definitions = $this->entityManager->getFieldDefinitions($this->entityType->id(), 'muuuh');
    

    nice ;)

    I think you're looking for getBaseFieldDefinitions() ?

  5. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,301 @@
    +      // @todo We should better just rely on information coming from the entity
    +      //   storage controller.
    

    no "controller"

  6. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,301 @@
    +          // @todo: may one field be in multiple tables, eg. revision + base?
    +          $this->mapFieldDefinition($field_name, $field_definitions[$field_name], $table_mapping, $data[$table]);
    

    data and revision data tables have mostly the same fields, and keys also exist in multiple of those, although I'm not sure how exactly you want to handle that?

  7. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,301 @@
    +    $field_schema = $this->fieldStorageDefinitions[$field_name]->getSchema();
    

    Is there a reason you're not using $field_definition->getFieldStorageDefinition() ?

  8. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,301 @@
    +    $views_field['title'] = $field_definition->getLabel() . " ($schema_field_name)";
    

    Is the field name here for a reason or just debugging? not sure that makes sense by default...

  9. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,301 @@
    +        // @todo No idea to determine how to find out whether the field is a number/string ID.
    

    $field_definition->getSetting('target_type') gives you the type, then you have 3 possibilities, config_entity => string, and for content_entity, you can get the field definition of the id key. EntityReferenceItem::getPropertyDefinitions() still has a @todo for that part, but it's implemented in ContentEntityDatabaseStorage::_fieldSqlSchema()

  10. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,301 @@
    +        // @todo Create an optional entity field handler, that can render the
    +        //   entity.
    

    I guess the options are similar to entity refernce formatters, id, label or rendered entity. Can we re-use the field views stuff and select the formatter you want to use?

dawehner’s picture

Status: Needs work » Needs review
FileSize
45.47 KB
28.69 KB

I guess we could introduce a description key on entity types?

default could be the entity label key?

Here is now a dedicated issue: #2323779: Add a description key to entities

nice ;)
I think you're looking for getBaseFieldDefinitions() ?

Much better!

Ah, that's how you do it, would it be easier to use += array() ?

Well, actually no, because the left side overrides the right side in a += expression, while we want to have it the other way round.

data and revision data tables have mostly the same fields, and keys also exist in multiple of those, although I'm not sure how exactly you want to handle that?

Well, I just do for now what getFieldNames() returns. I think it is fine, so we don't couple the views data to a specific implementation detail.

Is the field name here for a reason or just debugging? not sure that makes sense by default...

Well, if we want to expose for example the mimetype of a file we need to somehow put the additional column into the title. Any suggestions?

Is there a reason you're not using $field_definition->getFieldStorageDefinition() ?

Haven't adapted the code yet for this.

I guess the options are similar to entity refernce formatters, id, label or rendered entity. Can we re-use the field views stuff and select the formatter you want to use?

Good question. The fieldAPI field handler is so special, I would rather avoid extending from that class if possible.

Status: Needs review » Needs work

The last submitted patch, 71: 1740492-views_data-71.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
44.76 KB

Ha.

jhodgdon’s picture

Looks good! This is really elegant. Nice work!!

A few things I noticed:
a)

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -141,6 +141,18 @@ public function getKey($key);
   public function hasKey($key);
 
   /**
+   * Sets a specific entity key.
+   *
+   * @param string $key
+   *   The name of the entity key to return.
+   * @param string $value
+   *   The new value of the key.
+   *
+   * @return $this
+   */
+  public function setKey($key, $value);

The $key param should be "The name of the entity key to set a value for." right, not "to return"?

b) There are a couple of other methods/classes/etc. that have no doc blocks.

c)

+++ b/core/modules/file/src/Entity/File.php
- *     "views_data" = "Drupal\file\FileViewsData",
+ *     "views_data" = "Drupal\file\FileViewsData"

I was told removing trailing commas in annotation arrays is no longer necessary or desirable?

d) FileViewsData:

     // Sets 'group' index for file_managed table.
-    $data['file_managed']['table']['group']  = t('File');

If you're removing a line of code, probably remove the comment too? :)

e) Some thoughts on the default ViewsData controller, looking at what the File entity had to override... I'm wondering if the pieces needed by most entities would be cleaner if provided by protected class properties on the base ViewsData class? So for instance, there could be EntityViewsData::$baseHelp, and the getViewsData method would then set [table][base][help] to this value if set.

f) Regarding the @todo about field types... How about a class property, something like EntityViewsData::$fieldTypes, which would be an array whose keys are field data type machine names, and whose value is an array like:

array('field' => 'standard', 'filter' => 'standard', ...)

giving the handlers to use? This could both override and augment the hard-coded values in mapSingleFieldViewsData(). Seems like it would be fairly simple? Maybe you were already heading this way.

Berdir’s picture

e) As soon as we have the entity descriptions, see issue reference above, we can use that for the help text.

f) The problem we're trying to solve is that if contrib adds field types, they can integrate into this generation, and say that for my_fancy_field_type, it should us my_awesome_views_field plugin, and so on. We can't just put that in EntityViewsData, as those modules can't extend that. So the question is if it should be part of the plugin definition, or an optional method they can implement, or ...

jhodgdon’s picture

f) Right. I was suggesting that one easy way for doing this to use a member variable -- classes extending the base class can define a simple array-valued member variable, giving the handlers for each special field data type they have.

plach’s picture

Nice!

  1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -141,6 +141,18 @@ public function getKey($key);
    +  public function setKey($key, $value);
    

    Woot!

  2. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,318 @@
    +   * The storage controller used for this entity type.
    ...
    +  protected $storageController;
    

    Storage handler or just storage :)

  3. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,318 @@
    +      );
    +
    +    }
    +    if ($revision_table) {
    

    Surplus blank line

  4. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,318 @@
    +    $field_definitions = $this->entityManager->getBaseFieldDefinitions($this->entityType->id());
    

    #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions is going to provide more APIs for this.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -141,6 +141,18 @@ public function getKey($key);
    +   * @param string $value
    

    That could be mixed?

  2. +++ b/core/modules/file/src/FileViewsData.php
    @@ -495,8 +406,7 @@ public function getViewsData() {
    +    return NestedArray::mergeDeep($parent_views_data, $data);
    

    Why are we merging the values here instead of the usual way of just modifying the data array?

  3. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,318 @@
    +  protected function mapFieldDefinition($field_name, FieldDefinitionInterface $field_definition, TableMappingInterface $table_mapping, &$views_table_data) {
    

    docblock

  4. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,318 @@
    +    // @todo Allow field types to customize this.
    

    So we probably need to talk about this point then.

  5. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,318 @@
    +  public function getViewsTableForEntityType(EntityTypeInterface $entity_type) {
    

    docblock. Do we need this method? You think we might want to override this in extending classes?

jhodgdon’s picture

Also this base class needs a mention in the Entity API topic where it says to write your own Views data controller... in core/modules/system/entity.api.php around line 314:

 *   - views_data: A class implementing \Drupal\views\EntityViewsDataInterface
 *     to provide views data for the entity type.
 
Berdir’s picture

Title: Implement an entity views data controller » Implement a default entity views data handler

#76: Well, classes shouldn't have to know about the views integration of their field types, the field types should have to define that?

Updating issue title.

One problem here is that the changes in FileViewsData still have the table names hardcoded. So if someone decides to make file entity fields translatable or revisionable, it will still break. Can we find a way to avoid that? Maybe override the method that generates a single field definition and then a switch on the field name and add changes there?

jhodgdon’s picture

Berdir: in the File handler that is currently in the patch, it is overriding the Views handlers for several database columns related to files, such as Mime Type. In that case, it seems like the File entity has special knowledge that this particular database field, although it's just a string field in the entity base fields method, should be handled as a Views field with a special handler. So:

// File::baseFieldDefinitions() does:
    $fields['filemime'] = BaseFieldDefinition::create('string')
      ->setLabel(t('File MIME type'))
      ->setDescription(t("The file's MIME type."));

// FileViewsData::getViewsData() does:
    $data['file_managed']['filemime'] = array(
      'title' => t('Mime type'),
      'help' => t('The mime type of the file.'),
      'field' => array(
        'id' => 'file_filemime',
       ),

So it would seem useful for the File entity in its Views data handler class to be able to say something like
array('file_managed' => array('filemime' => array('field' => 'file_filemime')))
to say "The filemime column in file_managed: the field handler should be 'file_filemime' instead of the default Views handler for string fields".

Or maybe leave out the table name, so that it can just say "If you find a column named 'filemime' in any of my tables, use this handler'.

dawehner’s picture

FileSize
63.7 KB
35.18 KB

@berdir, @jhodgdon, @plach, @damiankloip

Thank you so much for all the reviews, discussion here. Really happy that this issue is actually in the interested of other people"

Woot!

Expanded the test coverage for that.

Storage handler or just storage :)

Yeah sorry, I am still really oldschool ;)

Surplus blank line

Ha, I agree, this looked annoying.

#1498720: Make the entity storage system handle changes in the entity and field schema definitions is going to provide more APIs for this.

This issue still feels like an explorative task, so yeah I would be happy about less work in this issue.

That could be mixed?

Theoretically I would agree, but getKey also returns a string.

Why are we merging the values here instead of the usual way of just modifying the data array?

Well, for sure we can find better ways to put things in. This is just way more convenience when you convert existing views data.

docblock

Meh, fixed

docblock. Do we need this method? You think we might want to override this in extending classes?

We (@jhodgdon and I) talked about using the data table as base table in views. This method would make it easy to extract this logic, but for sure,
we can just put that inline.

So we probably need to talk about this point then.

Yeah we need several adaptability on several levels:

  • You want to be able to extend the statis listing between data type and handler types
  • You might want to change the generated views data, depending on the "context", like data table, vs. revision table
  • You want to alter specific entries in the views data, like NodeViewsData/FileViewsDdata does now

Also this base class needs a mention in the Entity API topic where it says to write your own Views data controller... in core/modules/system/entity.api.php around line 314:

Improved

#76: Well, classes shouldn't have to know about the views integration of their field types, the field types should have to define that?

In an ideal world I would love to have "ViewsAwareItemInterface", but I guess we need something similar to constrains, so each field type (in core via an alter hook)
can define its matching views data integration.

Or maybe leave out the table name, so that it can just say "If you find a column named 'filemime' in any of my tables, use this handler'.

I guess the simplest way would be to provide the base table/ data table etc. as variables, you can reuse when you generate the views.

Status: Needs review » Needs work

The last submitted patch, 82: 1740492-views_data-82.patch, failed testing.

jhodgdon’s picture

Looks like there's an actual problem in this patch:

PHP Fatal error: Interface 'Drupal\Core\Entity\EntityControllerInterface' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/EntityViewsData.php on line 25

Anyway, that aside... Looking at the new Node implementation, some thoughts:

a)

       'filter' => array(
         'id' => 'bundle',
       ),

Could the bundle field be automatically detected in the base class?

b) In the interdiff for the base class main method:

-    $data = array();
+    $data = [];

Ugh... Why switch to this [] notation instead of array()? I don't think we're using this elsewhere in Core. :(((((

c) As a note... The Node implementation of this is going to be a problem with 'langcode'. In your base class, the langcode field is added no matter what, which is the right thing to do. But in the Node subclass, it is only modified if the 'language' module is enabled, which is what its data class is doing currently. Node shouldn't be doing that (we should have the fields/filters even if there is no Language module), but see #2320521: Follow-up: Node language views filters need label adjustments which is trying to remove that restriction -- tests fail because of another issue.

dawehner’s picture

Ugh... Why switch to this [] notation instead of array()? I don't think we're using this elsewhere in Core. :(((((

We started to use that in a couple of places already, so I switched to use it for new code everywhere. I actually think they improve the readability A LOT, especially for every
person which reads JSON or PYTHON from time to time.

Could the bundle field be automatically detected in the base class?

Good idea, totally right.

cosmicdreams’s picture

@dawehner there are ~ 3300 occurances of "= array();" in Drupal 8. There are 96 occurances of "= []".

If we want to use "= []" that's fine, let's just be consistent.

Should I open an issue for replacing every "= array();" with "= []". Would be a simple patch to execute on, but it would likely make every other patch conflict.

ParisLiakos’s picture

lets please not bikeshed the array syntax here..use this issue instead #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8
Using [] is perfectly valid, and we already use that numerous time in core

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.16 KB
64.45 KB

@jhodgdon
Thank you for your continuous feedback.

`Could the bundle field be automatically detected in the base class?

Expanded it! #2322949: Implement generic entity link view field handlers could make it even more generic.

c) As a note... The Node implementation of this is going to be a problem with 'langcode'. In your base class, the langcode field is added no matter what, which is the right thing to do. But in the Node subclass, it is only modified if the 'language' module is enabled, which is what its data class is doing currently. Node shouldn't be doing that (we should have the fields/filters even if there is no Language module), but see #2320521: Node views: Have language fields/filters even if no language module which is trying to remove that restriction -- tests fail because of another issue.

The new code always includes the languages filters ...

jhodgdon’s picture

Regarding Language module and Node -- this is still not going to work. Here's what's happening in the current patch:

a) In the base class, the langcode stuff is being added without a check for moduleExists('language'). This is correct.

b) In NodeViewsData::getViewsData(), it is only modifying the labels of the langcode fields if Language is enabled:

     if (\Drupal::moduleHandler()->moduleExists('language')) {

That is incorrect. This if() should be removed, because the fields are going to be there no matter what from the base class, so the labels need to be changed no matter what. Appears twice (once for the translation language,and once for the revision language, which is the original langauge field).

c) Currently there is a bug in Node creation, where if Language module is disabled, nodes are being created with the language set incorrectly. This is #1966436: Default *content* entity languages are not set for entities created with the API. When I tried to remove the if() statements in (b) in the NodeViewsData controller in #2320521: Follow-up: Node language views filters need label adjustments, quite a lot of tests failed because of this problem, and I would be very shocked if your latest patch does not have the same problem.

jhodgdon’s picture

Status: Needs review » Needs work

Other questions/comments about this latest patch -- mostly great!

a) From the Node class:

     $data['node_field_data']['table']['join']['node'] = array(
       'type' => 'INNER',

Is there a reason that some joins between the base table and data table should be left joins and others are inner joins?

b) From the Node class:

    $data['node_field_data']['title'] = array(
...
      'field' => array(
        // This is the real field which could be left out since it is the same.
        'field' => 'title',
        // This is the UI group which could be left out since it is the same.
        'group' => t('Content'),

These last 4 lines should just be removed, shouldn't they? This patch doesn't do it.

c) I'm noticing that on most of the Boolean fields, there are lines like:

         'output formats' => array(
           'sticky' => array(t('Sticky'), t('Not sticky')),
         ),
// and
         'type' => 'yes-no',

Maybe these should be part of the field definitions somehow? Possibly for a follow-up though.

d) On base tables:

       'defaults' => array(
         'field' => 'title',
       ),

Probably the Label field would be a sensible default for any entity?

e) It looks like the base class is handling the join between the node_field_revision table and node_revision, but not between node_field_revision and node?

f)

+    $parent_views_data = parent::getViewsData();
... 
+    return NestedArray::mergeDeep($parent_views_data, $data);

As someone commented above... Why not just do $data = parent::getViewsData() above, and then modify it directly and return $data? I don't see a reason to do a mergeDeep -- just seems like extra overhead?

g) Two methods on the base class lack docs:

+  protected function processViewsDataForLanguage($table, $field_name, array &$views_field) {
+  protected function processViewsDataForString($table, $field_name, array &$views_field) {

h) Does this really need to be a method?

+   * @return string
+   *   THe name of the base table in views.
+   */
+  protected function getViewsTableForEntityType(EntityTypeInterface $entity_type) {
+    return $entity_type->getBaseTable();
+  }
+  protected function getViewsTableForEntityType(EntityTypeInterface $entity_type) {
+    return $entity_type->getBaseTable();
+  }

If it does, there's a typo in the @return description (THe instead of The).

i) views.api.php

+ *   "views_data" annotation in the entity class. You can autogenerate big parts
+ *   of the ingration if you extend the \Drupal\views\EntityViewsData base
+ *   class. See the @link entity_api Entity API topic @endlink for more
+ *   information about entities.

Second line: ingration => integraation
but... shouldn't it say "You can autogenerate most of the views data by extending..."?

The last submitted patch, 88: 1740492-views_data-88.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
71.09 KB
20.9 KB

b) In NodeViewsData::getViewsData(), it is only modifying the labels of the langcode fields if Language is enabled:

Okay, sure let's fix that quick.

Is there a reason that some joins between the base table and data table should be left joins and others are inner joins?

I don't see a reason why it should not be there. Let's put that into the base class, to ensure that it is used everywhere.

These last 4 lines should just be removed, shouldn't they? This patch doesn't do it.

Well sure, I guess we simply cannot take the node as example any longer.

Maybe these should be part of the field definitions somehow? Possibly for a follow-up though.

Sure, this could be pulled up from lower levels of the system, but I would like to get crazy in this issue. It is already a quite big patch.

Probably the Label field would be a sensible default for any entity?

Good idea!!

As someone commented above... Why not just do $data = parent::getViewsData() above, and then modify it directly and return $data? I don't see a reason to do a mergeDeep -- just seems like extra overhead?

Well, okay, here is the patch which converts the full NodeViewsData. This is certainly a little bit more effort.

g) Two methods on the base class lack docs:

Fixed

h) Does this really need to be a method?

This was already answered above.

docblock. Do we need this method? You think we might want to override this in extending classes?
We (@jhodgdon and I) talked about using the data table as base table in views. This method would make it easy to extract this logic, but for sure,
we can just put that inline.

but... shouldn't it say "You can autogenerate most of the views data by extending..."?

Adapted.

Status: Needs review » Needs work

The last submitted patch, 92: 1740492-views_data-91.patch, failed testing.

jhodgdon’s picture

Um...

    $data['file_managed']['table']['base'] = array(
       // @TODO There is no corresponding information in entity metadata.
@@ -26,7 +26,7 @@ public function getViewsData() {
       'defaults' => array(
         'field' => 'filename'
       ),
-    );
+    ) + $data['file_managed']['table']['base'];

This is unnecessarily complicated, and is not what we've been asking for. Just set the values that need to be changed deep in the data array, right? It makes the patch a bit more complicated, but the resulting code cleaner...

Anyway, there's some kind of a PHP error in this patch...

jhodgdon’s picture

And if you don't want to do #94, I agree a single ArrayMergeDeep or whatever it is called is better than the array addition at every step.

Anyway... I'd really like to see this get done, because we have 4 multilingual views issues postponed on it. So far we don't have a working patch without PHP errors, and holding it up on array merging details seems silly (we can refactor the code later, and using array merge will keep the patch itself more readable). So can we get a working patch for Node and File, and go from there?

dawehner’s picture

I had my reasons that i went with the merge approach. I won't have time tonight but tomorrow is another day.

@jhodgon
It is great to momentum here. In general we can improve so many things, so we will have tons of additional work anyway.

Berdir’s picture

+++ b/core/modules/file/src/FileViewsData.php
@@ -62,61 +51,16 @@ public function getViewsData() {
 
     // Describes filename field in file_managed table.
-    $data['file_managed']['filename'] = array(
-      'title' => t('Name'),
-      'help' => t('The name of the file.'),
-      'field' => array(
-        'id' => 'file',
-       ),
-      'sort' => array(
-        'id' => 'standard',
-      ),
-      'filter' => array(
-        'id' => 'string',
-      ),
-      'argument' => array(
-        'id' => 'string',
-      ),
-    );
+    $data['file_managed']['filename']['field']['id'] = 'file';

There are a few of those now where the comment doesn't really match what we are doing anymore.

The advantage is that it's easier to review what we're doing, but the resulting code seems a bit strange.

Maybe we should put them all together with a single comment that says something like "Use file specific plugins and improve descriptions." or something like that?

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.11 KB
73.7 KB

This is unnecessarily complicated, and is not what we've been asking for. Just set the values that need to be changed deep in the data array, right? It makes the patch a bit more complicated, but the resulting code cleaner...

Okay, rewrote it again.

There are a few of those now where the comment doesn't really match what we are doing anymore.

The advantage is that it's easier to review what we're doing, but the resulting code seems a bit strange.

Maybe we should put them all together with a single comment that says something like "Use file specific plugins and improve descriptions." or something like that?

True, I think these comments aren't helpful at all.

Status: Needs review » Needs work

The last submitted patch, 98: views_data-1740492-97.patch, failed testing.

jhodgdon’s picture

Thanks! The code reads a lot better in this version... will wait for a patch that passes tests to review fully.

dawehner’s picture

Status: Needs work » Needs review
FileSize
73.79 KB
4 KB

Beside of language problems, this should be it.

Status: Needs review » Needs work

The last submitted patch, 101: 1740492-views_data-101.patch, failed testing.

jhodgdon’s picture

The language problems should be gone now, I think? There seems to be something else going on in these test failures?

jhodgdon’s picture

Oh wait I will hit retest because I think the commit to #1966436: Default *content* entity languages are not set for entities created with the API happened after this test run.

The last submitted patch, 101: 1740492-views_data-101.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
75.17 KB
7.02 KB

@jhodgdon
The failures are all now all caused by the following SQL query caused by the frontpage view:

'SELECT node_field_data.sticky AS node_field_data_sticky, node_field_data.created AS node_field_data_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode
FROM 
{node} node
INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
WHERE (( (node_field_data.promote <> 0) AND (node_field_data.status = 1) AND (node_field_data.langcode IN  ('en')) ))
ORDER BY node_field_data_sticky DESC, node_field_data_created DESC'

Most tests though create nodes with language undefined (this is also ensured by #1966436: Default *content* entity languages are not set for entities created with the API).
Is it just me or should the content language on a non multilingual site be rather
"unspecified" than 'en' by default?

Status: Needs review » Needs work

The last submitted patch, 107: views_data-1740492-107.patch, failed testing.

Gábor Hojtsy’s picture

@dawehner / #107: Content language on a non-foreign language site better be English by default, because when you add German next, you are up for a migration process otherwise. If the existing content says "no language", then how do we tell which is actually English and which is actually "no language"? Similar for a German-only site where German is the only language you have, once you enable some other language, how do we tell which content is German if it was not all originally marked German? So the D8 idea is all content will have an explicit language unless otherwise instructed.

On the the patch itself, it is quite a beast. I tried to get an overview but the summary is unfortunately very short and I could not understand what was in scope for this issue vs. what this issue enables, etc. So needs an issue summary update AFAIS.

dawehner’s picture

Issue summary: View changes

@dawehner / #107: Content language on a non-foreign language site better be English by default, because when you add German next, you are up for a migration process otherwise. If the existing content says "no language", then how do we tell which is actually English and which is actually "no language"? Similar for a German-only site where German is the only language you have, once you enable some other language, how do we tell which content is German if it was not all originally marked German? So the D8 idea is all content will have an explicit language unless otherwise instructed.

Okay sure, but then can you explain why all those nodes actually marked as language undefined?

On the the patch itself, it is quite a beast. I tried to get an overview but the summary is unfortunately very short and I could not understand what was in scope for this issue vs. what this issue enables, etc. So needs an issue summary update AFAIS.

Improved the IS a bit. In case you wonder about the setKey method, it is used in the patch in order to make the testing nicer. I think it just makes sense to add it.

On the the patch itself, it is quite a beast. I tried to get an overview but the summary is unfortunately very short and I could not understand what was in scope for this issue vs. what this issue enables, etc. So needs an issue summary update AFAIS.

Do you have some actual concrete questions? This is one of these issues which noones reviews anyway.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
79.67 KB

Let's see how many issues fixes creating all nodes as 'en'

dawehner’s picture

FileSize
80.38 KB
733 bytes

One more fix, I cannot get my head around the rdf tests though.

The last submitted patch, 111: views_data-1740492-111.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 112: views_data-1740492-112.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.15 KB
82.53 KB

Fixed the remaining failures

Status: Needs review » Needs work

The last submitted patch, 115: views_data-1740492-115.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
83.36 KB
853 bytes

There we go.

Berdir’s picture

Two problems...

a) The default view should really show content of the default language *and* unspecified, otherwise that would never be shown and there is also no risk for duplicates, as unspecified can't have translations.
b) drupalCreateNode() still incorrectly hardcodes the default value for langcode to undefined. See #2333731: WebTestBase::drupalCreateNode() should not hardcode default values

Status: Needs review » Needs work

The last submitted patch, 117: views_data-1740492-117.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
83.83 KB
478 bytes

fixed that failure as well.

jhodgdon’s picture

Status: Needs review » Needs work

I have looked carefully through this patch.

I'm confused as to why we would want the Translation Link pseudo-field added to Node if the Content Translation module is disabled -- and would the handler even be defined in this case?

-    if (\Drupal::moduleHandler()->moduleExists('content_translation')) {
-      $data['node']['translation_link'] = array(
...

vs.

+    $data['node']['translation_link'] = array(
+      'title' => t('Translation link'),

Also, regarding #118, it looks like the default language for nodes on WebTestBase is now OK, so all the lines like

-    $this->node = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1, 'uid' => $this->web_user->id()));
+    $this->node = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1, 'uid' => $this->web_user->id(), 'langcode' => 'en'));

in the patch should not be necessary. Hopefully!

And sorry for not reviewing sooner -- I was on a bike trip. Back now. Paying attention. Let's get this done!

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs issue summary update
FileSize
74.63 KB
9.79 KB

@jhodgdon
Good catch, probably happened as the if() included more code earlier.

in the patch should not be necessary. Hopefully!

Yeah indeed they aren't, berdir++

And sorry for not reviewing sooner -- I was on a bike trip. Back now. Paying attention. Let's get this done!

No problem, you can always work on other issues anyway.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK, I think this is ready to go!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -141,6 +141,18 @@ public function getKey($key);
       /**
    +   * Sets a specific entity key.
    +   *
    +   * @param string $key
    +   *   The name of the entity key.
    +   * @param string $value
    +   *   The new value of the key.
    +   *
    +   * @return $this
    +   */
    +  public function setKey($key, $value);
    

    I'm not a huge fan of putting a method on a public API that is only used in tests.

  2. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,444 @@
    +use Drupal\Core\Entity\ContentEntityInterface;
    

    Not used

  3. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,444 @@
    +
    +  /**
    +   * Constructs an EntityViewsData object.
    +   *
    +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    +   *   The entity type to provide views integration for.
    +   * @param \Drupal\Core\Entity\Sql\SqlEntityStorageInterface $storage_controller
    +   *   The storage controller used for this entity type.
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    +   *   The entity manager.
    +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    +   *   The module handler.
    +   * @param \Drupal\Core\StringTranslation\TranslationInterface $translation_manager
    +   *   The translation manager.
    +   */
    +  function __construct(EntityTypeInterface $entity_type, SqlEntityStorageInterface $storage_controller, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, TranslationInterface $translation_manager, TypedDataManager $typed_data_manager) {
    

    doc block does not match constructor.

  4. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,444 @@
    +  /**
    +   * Puts the views data for a single field onto the views data.
    +   *
    +   * @param string $field_name
    +   *   The name of the field to handle.
    +   * @param \Drupal\Core\Field\FieldDefinitionInterface $field_definition
    +   *   The field definition defined in Entity::baseFieldDefinitions()
    +   * @param \Drupal\Core\Entity\Sql\TableMappingInterface $table_mapping
    +   *   The table mapping information
    +   * @param array $table_data
    +   *   A reference to a specific entity table (for example data_table) inside
    +   *   the views data.
    +   */
    +  protected function mapFieldDefinition($table, $field_name, FieldDefinitionInterface $field_definition, TableMappingInterface $table_mapping, &$table_data) {
    

    Missing $table from docblock. Also why pass the field name if you are passing the field definition?

  5. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,444 @@
    +  /**
    +   * Provides the views data for a given data type and schema field.
    +   *
    +   * @param string $data_type
    +   *   The data type to generate views data for, for example "int". The data
    +   *   type comes directly from the schema definition of each field item.
    +   * @param string $schema_field_name
    +   *   The schema field name.
    +   * @param bool $first
    +   *   Is it the first column of the schema.
    +   *
    +   * @return array
    +   *   The modified views data field definition.
    +   */
    +  protected function mapSingleFieldViewsData($table, $field_name, $data_type, $schema_field_name, FieldDefinitionInterface $field_definition, $first) {
    

    Missing $table, $field_name, $field_definition from @param. Also why passed field_name if you are passing the definition.

  6. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,444 @@
    +  /**
    +   * Processes the views data for a language field.
    +   *
    +   * @param string $table
    +   *   The table the language field is added to.
    +   * @param string $field_name
    +   *   The field name of the language field.
    +   * @param array $views_field
    +   *   The views field data.
    +   */
    +  protected function processViewsDataForLanguage($table, $field_name, FieldDefinitionInterface $field_definition, array &$views_field) {
    

    $field_definition is not used and not documented - does it need to be here?

  7. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -0,0 +1,444 @@
    +  /**
    +   * Processes the views data for an entity reference field.
    +   *
    +   * @param string $table
    +   *   The table the language field is added to.
    +   * @param string $field_name
    +   *   The field name of the language field.
    +   * @param array $views_field
    +   *   The views field data.
    +   */
    +  protected function processViewsDataForEntityReference($table, $field_name, FieldDefinitionInterface $field_definition, array &$views_field) {
    

    Missing $field_definition and do we need $field_name if we are passing in the definition?

  8. +++ b/core/modules/views/tests/Drupal/views/Tests/EntityViewsDataTest.php
    @@ -0,0 +1,626 @@
    +   * @var \Drupal\Core\Entity\ContentEntityDatabaseStorage|\PHPUnit_Framework_MockObject_MockObject
    

    ContentEntityDatabaseStorage does not exist anymore

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
73.33 KB
14.86 KB

I'm not a huge fan of putting a method on a public API that is only used in tests.

Okay, let's adapt it.

Not used

Dropped

doc block does not match constructor.

Fixed

Missing $table from docblock. Also why pass the field name if you are passing the field definition?

Yeah $field_name is indeed not needed, good point!

$field_definition is not used and not documented - does it need to be here?

These process methods all retrieve the same arguments.

ContentEntityDatabaseStorage does not exist anymore

Adapted.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

In mapSingleFieldViewsData(), the added @param does not have a description.

alexpott’s picture

+++ b/core/modules/node/src/NodeViewsData.php
@@ -264,6 +117,7 @@ public function getViewsData() {
+    // @Todo Add similar support to any date field.

@@ -402,83 +241,36 @@ public function getViewsData() {
+    // @todo the NID field needs different behaviour on revision/non-revision

+++ b/core/modules/views/src/EntityViewsData.php
@@ -0,0 +1,439 @@
+    // @todo In theory we should use the data table as base table, as this would
+    //   save one pointless join (and one more for every relationship).
...
+      // @todo We should better just rely on information coming from the entity
+      //   storage.
...
+      // @todo Introduce a concept of the "main" schema field for a field item.
+      //   This would be the FID for a file reference for example.
...
+    // @todo Allow field types to customize this.
...
+        // @todo Should the actual field handler respect that this is just renders a number
+        // @todo Create an optional entity field handler, that can render the
+        //   entity.
...
+   * @todo Given that the base_table is pretty much useless as you often have to
+   *   join to the data table anyway, it could make a lot of sense to start with
+   *   the data table right from the beginning.

+++ b/core/modules/views/tests/Drupal/views/Tests/EntityViewsDataTest.php
@@ -0,0 +1,659 @@
+    // @todo Can we provide additional support for UUIDs in views?

@todo's with no issues... catch has a quote about this...

Sorry I should have noticed before.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @dawehner - it's not really holding up the issue and it's good practice.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I think the changes between are #125and #128 are minor enough for me to rtbc and commit.

Committed 35b3d0f and pushed to 8.0.x. Thanks!

  • alexpott committed 35b3d0f on 8.0.x
    Issue #1740492 by dawehner, damiankloip, dasjo, xjm: Implement a default...
jhodgdon’s picture

Annnnnnd there was much rejoicing! Thanks alexpott! 4 blocked issues unpostponed.

dawehner’s picture

Nice, thank you for all the reviews!

tim.plunkett’s picture

jhodgdon’s picture

We also got the labels for the language fields slightly confused. See #2320521: Follow-up: Node language views filters need label adjustments.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jibran’s picture

+++ b/core/modules/views/src/EntityViewsData.php
@@ -0,0 +1,446 @@
+    $process_method = 'processViewsDataFor' . Container::camelize($data_type);
+    if (method_exists($this, $process_method)) {
+      $this->{$process_method}($table, $field_definition, $views_field);
+    }

How would I do this for DER? I can't create processViewsDataForDynamicEntityReference. See #2548395-4: Provide views relationships for DER base fields for further discussion.