Problem

  • field_views_field_default_views_data() tries to register views data for entities that do not have a base table.

Steps to reproduce

  1. Enable Contact module.
  2. Enable Views module.
CommentFileSizeAuthor
#24 1867572-24.patch3.21 KBdamiankloip
#24 interdiff.txt670 bytesdamiankloip
#20 drupal-1867572-20.patch3.22 KBdawehner
#20 interdiff.txt859 bytesdawehner
#17 drupal-1867572-tests-only.patch1.89 KBdawehner
#17 drupal-1867572-14.patch2.87 KBdawehner
#15 drupal-1867572-15.patch3.18 KBdamiankloip
#14 interdiff.txt1.15 KBdamiankloip
#14 drupal-1867572-14.patch3.17 KBdamiankloip
#11 interdiff.txt936 bytesdawehner
#11 drupal-1867572-11.patch2.82 KBdawehner
#10 1867572-9.patch2.82 KBswentel
#10 interdiff.txt905 bytesswentel
#2 drupal-1867572-2-tests.patch1.51 KBdawehner
#2 drupal-1867572-2.patch2.79 KBdawehner
drupal8.field-views-data-base-table.0.patch1001 bytessun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Oh i see contact messages aren't actually stored at all, but they are fieldable.

Wouldn't it make maybe more sense to check whether the entity has a storage engine and this engine extends the database?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
drupal-1867572-2.patch2.79 KB
drupal-1867572-2-tests.patch1.51 KB

On the longrun we probably want to have a MemoryStorageController.

Here comes a test + the patch and test.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, drupal-1867572-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
Issue tags: +VDC

#2: drupal-1867572-2.patch queued for re-testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Whatever contact module is up, it seems a bit wrong anyway. This fix works fine, and has tests. Ideally it wouldn't be needed but meh :/

This might need to be re rolled for the new field api changes?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: -VDC

#2: drupal-1867572-2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1867572-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

#2: drupal-1867572-2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, drupal-1867572-2.patch, failed testing.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs work » Needs review
FileSize
interdiff.txt905 bytes
1867572-9.patch2.82 KB

This should fix it

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
drupal-1867572-11.patch2.82 KB
interdiff.txt936 bytes

We could save some time my not calling ->get() without some arguments.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

Yep, why not. Many savings of time will lead to lots of time eventually :)

olli’s picture

olli CreditAttribution: olli commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/contact/lib/Drupal/contact/Tests/Views/ContactFieldsTest.php
@@ -0,0 +1,65 @@
+    $data = drupal_container()->get('views.views_data')->get($this->field['field_name']);

    Should we pass the table, not field name?

  2. The latest tests-only-patch in #2 has "0 fail(s)".
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
drupal-1867572-14.patch3.17 KB
interdiff.txt1.15 KB

Good spot. We do need to prefix the field name, yes. Otherwise we return ALL the data. SO the test at the moment is actually a false pass :) Let's add a test to check we have the expected data array back aswell, or is that too much?

I think yched had a really good point a little while ago that views_data returns everything if a key isn't found.

Also, did you check out the tests from #2, the exceptions are right I think, as they throw errors that initially were the problem trying to join on a non existent base table.

How about this? Interdiff is slightly wrong as I added too early, but you get the idea.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
drupal-1867572-15.patch3.18 KB

With a correct assertion message for the test too.

olli’s picture

olli CreditAttribution: olli commented
Status: Needs review » Reviewed & tested by the community

Great, thanks. I dont know if checking the keys is too much.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Reviewed & tested by the community » Needs review
FileSize
drupal-1867572-14.patch2.87 KB
drupal-1867572-tests-only.patch1.89 KB

Good points, here are new versions of the actual patch/test only.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

I also just created this related issue: #1880834: Don't return all data from views_data service when an invalid table is requested

olli’s picture

olli CreditAttribution: olli commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/contact/lib/Drupal/contact/Tests/Views/ContactFieldsTest.php
@@ -0,0 +1,66 @@
+    $table_name = _field_sql_storage_tablename($this->field);
+    $data = drupal_container()->get('views.views_data')->get($table_name);

That is even better. Could we roll #15 with it?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt859 bytes
drupal-1867572-20.patch3.22 KB

Sorry damian, I forgot to refresh the browser window before working on stuff.

This interdiff is between this patch and #15

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

No worries, looks good to me. I guess we might as well use the field helper function to get that.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

RTBC as per #19

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1867572-20.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt670 bytes
1867572-24.patch3.21 KB

We want to assign $this->field the return value of field_create_field, so we get all the defaults.

This should be good now.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Looks good for me!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Fixed » Active

Mmm... why was this test added to Contact module?

Test coverage should always be added to the component/extension/code that exposes functionality and that is responsible for ensuring that its own functionality works as expected.

It's not Contact module's responsibility to test that Views operates correctly on entities that are not stored. Contact module really doesn't have any business with Views right now - like, none at all. ;)

Let's move this test over to Views' tests.

Also, we probably want to add an entity type to entity_test that doesn't have any storage, instead of adding a cross-test-dependency on Contact module. Just omitting the 'base_table' declaration in the entity type annotation should trigger the exact problem identically to Contact module's Message entity.

olli’s picture

olli CreditAttribution: olli commented

Good point. Would it make any sense to move it to field module instead where the fix is, say Drupal\field\Tests\Views\ApiDataTest?

swentel’s picture

swentel CreditAttribution: swentel commented
Title: Field module generates views data for entities without base table » [follow up] Field module generates views data for entities without base table
Category: bug » task

This is not a bug anymore. If someone wants to move go ahead.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 579c3ba on 8.3.x 
    Issue #1867572 by swentel, dawehner, damiankloip, sun: Fixed Field...

  • webchick committed 579c3ba on 8.3.x 
    Issue #1867572 by swentel, dawehner, damiankloip, sun: Fixed Field...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 579c3ba on 8.4.x 
    Issue #1867572 by swentel, dawehner, damiankloip, sun: Fixed Field...

  • webchick committed 579c3ba on 8.4.x 
    Issue #1867572 by swentel, dawehner, damiankloip, sun: Fixed Field...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Title: [follow up] Field module generates views data for entities without base table » Field module generates views data for entities without base table
Category: Task » Bug report
Issue summary: View changes
Status: Active » Fixed
Issue tags: +Bug Smash Initiative

This was committed and re-open for follow up work. I've moved that to a new issue, #3270033: Move ContactFieldsTest.php to the views module and decouple it from contact module.

Therefore the fixed status from #26 can be restored as well as the Bug category.

Status: Fixed » Closed (fixed)

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