Problem/Motivation

The views data structure currently directly looks like the table structure.
This works fine for the SQL query plugin, but it is problematic for differnet problems

  • You cannot write a generic handler, it is ALWAYS coupled to SQL. Per default
    it should rather be possible to not couple to SQL but rather just couple in case
    you need the flexibility
  • Updating entity metadata changes the table structure, so the configured views
    would have to be updated on the fly
  • For people implementing EQ_views would need another totally separate views data
    structure, so also the configured views don't work together at all.

Another problem we have is that in case the entity schema changes, the views data schema changes at the same time.
At the same time the views configuration keeps the same, we end up with broken views, see #2341323: Adapt the references field / table names in views, when corresponding entity schema changes

Proposed resolution

As a first step, store the used entity types/entity fields in the configuration so we can use it both for potential better
views querying, but more important for now, use it in case we change the entity schema.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review
FileSize
2.09 KB

So IMO the first baby step is making sure our tables know the entity they are form. Currently this is only statically added to the base table.

Status: Needs review » Needs work

The last submitted patch, 1: 2349553.patch, failed testing.

xjm’s picture

Priority: Normal » Major
dawehner’s picture

Status: Needs work » Needs review
FileSize
13.68 KB
11.81 KB

So this is the first bit, add the needed information (entity type + field name) onto the views data.

dawehner’s picture

FileSize
18.67 KB
4.91 KB

Expanded the unit test coverage a bit. It would be great to get just this bit in, once we have done this, we could
both experiment with the automatic updating of the views on schema change and iterate on the base schema.

dawehner’s picture

FileSize
7.89 KB
26.02 KB

Some more work.

catch’s picture

dawehner’s picture

Priority: Major » Critical

True.

jhodgdon’s picture

I took a look at the latest patch.

I'm a bit confused about one thing. It looks like both the 'entity_type' and 'entity_field' array elements are expected to be on all views field entries in the data definition -- various parts of the patch are looking for them, at least, and it's been added to the schema, I think.

But then in the EntityViewsData::mapFieldDefinition part of the patch, it seems to be only adding 'entity_field' to field entries:

+      $table_data[$views_field_name]['entity field'] = $field_name;

While the 'entity_type' part is added at the table level in the getViewsData() method:

+      $table_data['table']['entity type'] = $entity_type_id;

I am not seeing that addition in the schema?

And then there is this very confusing (probably wrong) bit in ViewsExecutable:

+    if (isset($data['table']['entity type'])) {
+      $fields[$id]['entity_type'] = $data['table']['entity type'];
+    }
+    if (isset($data[$field]['entity field'])) {
+      $fields[$id]['entity_field'] = $data['table']['entity type'];
+    }
+

This seems to be setting an 'entity_field' entry on a field to the table's entity type??!? That is probably a typo? The fact that all the tests pass with this in there is a bit worrisome.

dawehner’s picture

Thank you for your feedback!

I'm a bit confused about one thing. It looks like both the 'entity_type' and 'entity_field' array elements are expected to be on all views field entries in the data definition -- various parts of the patch are looking for them, at least, and it's been added to the schema, I think.

Never said that this patch is perfect, I just want to keep momentum in general.

This seems to be setting an 'entity_field' entry on a field to the table's entity type??!? That is probably a typo? The fact that all the tests pass with this in there is a bit worrisome.

@see previous answer :) In order words, don't trust me :)

jhodgdon’s picture

Status: Needs review » Needs work

OK then, setting status appropriately, it looks like we need a new patch?

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.67 KB
38.69 KB

Worked a bit on it, fixed the critical parts of the review.
I decided to make the views data itself as small as possible but just blow up the actual views config.

Wrote some tests to ensure the information is there at least.

To be honest the current patch primarily helps #2341323: Adapt the references field / table names in views, when corresponding entity schema changes

Status: Needs review » Needs work

The last submitted patch, 12: 2349553-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.63 KB
41.53 KB

One less test failure, hopefully.

Status: Needs review » Needs work

The last submitted patch, 14: 2349553-14.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
45.05 KB
3.52 KB

Fixed the remaining failures.

jhodgdon’s picture

This looks good!

So, I'm still a bit confused about one thing. The schema change in views.data_types.schema.yml adds both entity_type and entity_field properties to the views_handler schema definition. This means it applies to everything has a handler (area, field, sort, filter, argument).

But looking at all of the code changes, it seems like it really only applies to field handlers.

Is that a problem?

Also I think this change would need to be documented somewhere. We have an open issue about improving the documentation of hook_views_data() somewhere... which has gone nowhere due I think to lack of reviews and rerolls... but this change needs to update the hook docs so that it explains this new requirement.

dawehner’s picture

Thank you for your feedback!

So, I'm still a bit confused about one thing. The schema change in views.data_types.schema.yml adds both entity_type and entity_field properties to the views_handler schema definition. This means it applies to everything has a handler (area, field, sort, filter, argument).

Well, the patch is not done yet, we have to potential update all of the existing views still, so it will be applied to filter etc. as well.

dawehner’s picture

FileSize
5.82 KB
159.1 KB
112.67 KB

Alright, so I decided to write a script which updates all existing views (see update-views.php.txt)

Status: Needs review » Needs work

The last submitted patch, 19: 2349553-19.patch, failed testing.

xjm’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
159.13 KB

Just merged.

Status: Needs review » Needs work

The last submitted patch, 22: 2349553-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
158.23 KB

Just merged

... and produced a merge error ...

fago’s picture

Status: Needs review » Needs work

I think in the end we should build up the views data "table" structure based upon virtual entity fields only, but that's too enthusiastic for now. Encoding the metadata in the table structure is really good first step in this direction and helps to solve current problems, so +1 on that!

Reviews the patch - interesting to see that it also moves node.title to node_field_data from node - but that's fine and is probably caused by view regeneration? Else, I found some small glitches:

  1. +++ b/core/modules/views/tests/src/Unit/Plugin/field/CounterTest.php
    @@ -72,7 +72,10 @@ protected function setUp() {
         $storage = new View($config, 'view');
    
    +++ b/core/modules/views/tests/src/Unit/ViewExecutableFactoryTest.php
    @@ -47,6 +47,13 @@ class ViewExecutableFactoryTest extends UnitTestCase {
       protected $viewExecutableFactory;
    
    +++ b/core/modules/views/tests/src/Unit/ViewExecutableUnitTest.php
    @@ -78,4 +116,192 @@ public function testBuildThemeFunctions() {
    +
    ...
    +
    +
    

    Unnecessary new lines.

  2. +++ b/core/modules/views/tests/src/Unit/ViewsHandlerManagerTest.php
    @@ -30,15 +30,38 @@ class ViewsHandlerManagerTest extends UnitTestCase {
    +
    +  protected function setupMockedFactory() {
    

    Missing docs.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Ghent DA sprint
FileSize
1.28 KB
158.28 KB

Reviews the patch - interesting to see that it also moves node.title to node_field_data from node - but that's fine and is probably caused by view regeneration?

Good catch! As explained personally, that was an existing bug in HEAD, so I fixed that.

dawehner’s picture

Title: Views data structure » Store which fields in the views configuration
Issue summary: View changes

Update the IS

pfrenssen’s picture

Reviewed the script from #19 that was used to generate the patch.

  • I used the following one liner to find all tables to compare them with $table_entity_map:
    $ find core/ -iname views.view.*.yml -exec grep '[[:space:]]table:' {} \; | sort | uniq | sed "s|[[:space:]]*table: ||g"
    

    This finds entity_test__field_test and taxonomy_term_hierarchy, but both of these are fine, as they are describing a relationship and not a field. The oneliner cannot distinguish between those.

  • All the skipped tables look good.
  •   // is this right?
      'users:uid_raw' => NULL,
    

    This is a field alias, running users:uid through the Numeric filter plugin. Assuming this is correct, it would get its data from users:uid which is correctly converted.

  • Logic in convert_single_config() looks solid. It is only skipping the creation of entity_type and entity_field if there is no table, or the table has been blacklisted in $table_entity_map which I verified.

All systems go!

pfrenssen’s picture

This is a version of the patch from #26 excluding all the automatically generated config changes. This is a bit easier to review.

$ git diff origin/8.0.x -- $(git diff origin/8.0.x --name-only | grep -v "views\.view\.")
jhodgdon’s picture

Title: Store which fields in the views configuration » Store entity field information in the views data

New issue title made no sense to me... how about this one?

jhodgdon’s picture

Looking at the latest patch... I have a few comments and questions:

a) In the automated part where the views configs are being added to, I'm a bit confused at some places where there is just entity_type added and not entity_field. Can you explain that? For instance:

+++ b/core/modules/content_translation/tests/modules/content_translation_test_views/test_views/views.view.test_entity_translations_link.yml
...
@@ -69,6 +71,7 @@ display:
           hide_alter_empty: true
           text: Translate
           plugin_id: content_translation_link
+          entity_type: user
       filters:

Also

+++ b/core/modules/node/config/install/views.view.frontpage.yml
@@ -45,6 +45,7 @@ display:
           relationship: none
           table: node
           plugin_id: node_listing_empty
+          entity_type: node

It seems like the view handler stuff should either have both entity_type and entity_field, or neither, not just entity_type -- what good would that do?

b) I'm also curious whether the script in #19 that updated the views configs produces the same result as if the view was re-created and exported from the Config module UI in all cases. I guess it would be interesting as a test to try at least one or two and verify? This patch is huge...

c) Another question I have about this patch is whether the Wizard code changes are sufficient. For instance, in the Node wizard, the only changes in the patch are in the defaultDisplayOptions() method. But this wizard also sets up filters in defaultDisplayFiltersUser(), and also sets up the title field in display_options_row(). Neither of these have the entity_type and entity_field added. Shouldn't they?

I think there are probably other places in other Wizard classes with the same problems (if this is actually a problem; I am not sure).

d) EntityViewsDataTest::assertField() might be more convincing if it also verified the entity_type was correct. That may also be missing from some of the other tests.

dawehner’s picture

FileSize
11.66 KB
163.21 KB

Thank you for your detailed review!

a) In the automated part where the views configs are being added to, I'm a bit confused at some places where there is just entity_type added and not entity_field. Can you explain that? For instance:

I do agree that this is not obvious. The reason why we don't have 'entity_field' here, is that we have just custom fields link the link fields,
which aren't real existing fields, but still exists on the corresponding table in hook_views_data().

It seems like the view handler stuff should either have both entity_type and entity_field, or neither, not just entity_type -- what good would that do?

I could certainly agree that renaming the base table is more an esoteric usecase, and we don't have any kind of support for that anyway yet. Do you think those examples should be dropped again, even we might in an ideal future, work on it?

One counter example is comment_views_data.node ... which provides a relationship from the comment to the linked entity. That one is on the data table,
which could be also dropped/renamed at some point.

b) I'm also curious whether the script in #19 that updated the views configs produces the same result as if the view was re-created and exported from the Config module UI in all cases. I guess it would be interesting as a test to try at least one or two and verify? This patch is huge...

Well, ... its a general problem, that the test views can't be rexported like that, because they rely on schema etc. which is sometimes manually/dynamically defined. @alexpott though for example has a script which resaves the existing default views, and this got applied at least once
for the various views config schema issues.

c) Another question I have about this patch is whether the Wizard code changes are sufficient. For instance, in the Node wizard, the only changes in the patch are in the defaultDisplayOptions() method. But this wizard also sets up filters in defaultDisplayFiltersUser(), and also sets up the title field in display_options_row(). Neither of these have the entity_type and entity_field added. Shouldn't they?

You are absolute right that this was not completed, damn wizard code. #2383157: Let views wizards use ViewExecutable::addHandler is intended to solve it in a way that we are keeping sane in the future. Let's add some dedicated test coverage here.

d) EntityViewsDataTest::assertField() might be more convincing if it also verified the entity_type was correct. That may also be missing from some of the other tests.

Well, ... the entity table is stored on the outer levels, so we have test coverage for that already.

    $this->assertEquals('entity_test', $data['entity_test']['table']['entity type']);
    $this->assertEquals('entity_test_mul', $data['entity_test_mul_property_data']['table']['entity type']);
    $this->assertEquals('entity_test_mulrev', $data['entity_test_mulrev_revision']['table']['entity type']);
    $this->assertEquals('entity_test_mulrev', $data['entity_test_mulrev_property_revision']['table']['entity type']);

Status: Needs review » Needs work

The last submitted patch, 32: 2349553-32.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
163.83 KB
1.07 KB

Ha, we had a wrong schema!

jhodgdon’s picture

RE #31(a), I am not in favor of adding things "in case we might need them later". So I think adding ['entity_table'] without ['entity_field'] to the things that aren't real fields is not a good idea now, if we don't need it now. It certainly let me to be confused about them!

RE point (d), if the field handler info needs to have ['entity_table'] defined on it, then let's test for it as well as for the presence of ['entity_field']. If it doesn't need this... but I think it does? ... then let's not put it there at all?

Also the interdiff uploaded in #32 seems to be from the wrong issue. :( and the interdiff utility failed on me :( too bad.

dawehner’s picture

Also the interdiff uploaded in #32 seems to be from the wrong issue. :( and the interdiff utility failed on me :( too bad.

I'm sorry

RE #31(a), I am not in favor of adding things "in case we might need them later". So I think adding ['entity_table'] without ['entity_field'] to the things that aren't real fields is not a good idea now, if we don't need it now. It certainly let me to be confused about them!

Haha, well, the full point of this issue is to make #2341323: Adapt the references field / table names in views, when corresponding entity schema changes possible ...

RE point (d), if the field handler info needs to have ['entity_table'] defined on it, then let's test for it as well as for the presence of ['entity_field']. If it doesn't need this... but I think it does? ... then let's not put it there at all?

... we do have test coverage for that already, as written above. It would also make the assertion way worse to read,
because you would need more parameters and what not.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed the patch haven't found anything questionable so RTBC

Status: Reviewed & tested by the community » Needs work

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

fago’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/core/modules/views/src/ViewExecutable.php
@@ -1526,9 +1536,7 @@ public function attachDisplays() {
-      // Create a clone for the attachments to manipulate. 'static' refers to the current class name.
-      $cloned_view = new static($this->storage, $this->user);
-      $cloned_view->setRequest($this->getRequest());
+      $cloned_view = Views::executableFactory()->get($this->storage);

No idea what this change is about. Else, looks good to go to me else.

pfrenssen’s picture

Status: Reviewed & tested by the community » Needs work

Test bot disagrees.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
164.07 KB
larowlan’s picture

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

dawehner’s picture

No idea what this change is about. Else, looks good to go to me else.

Just in case someone is interested ... given that we have a factory we should just leverage that here, when we initialize another view, even
if we are part of the view executable already.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 7cd91c9 on 8.0.x
    Issue #2349553 by dawehner, pfrenssen, damiankloip: Store entity field...

Status: Fixed » Closed (fixed)

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