Problem/Motivation

The same error exists in D7. This is a port of the patch posted by jhedstrom in the related issue.
In the following scenarios this error occurs when viewing the page created with a view:
Fatal error: Call to a member function ensureMyTable() on a non-object

Scenarios:

  1. Add fields to the combined field filter, then remove a field from that display.
  2. Add fields to the combined field filter, make a second display, override the field settings for this display and remove a field.
  3. The original D7 issue report was due to fields not being available to certain users because of field access modules. I have no idea if this available in D8 yet, but I imagine it will be.
  4. Deleting a field from an entity that is used in the filter, gives error when searching.

I've attached a view export of a view that will give this error.

Proposed resolution

After applying the patch, the filter handler tests if a field used still exists in the current display, and if not, skips the field
and renders nothing at the end. It is a misconfiguration, so we should stop rendering.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, because it breaks drupal
Issue priority Normal, because it is not a often used feature
Disruption No disruption at all
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -61,6 +61,11 @@ public function query() {
+      //overwitten fields can lead to fields missing from a display that are still
+      //set in the non-overwritten combined filter
+      if (!isset($this->view->field[$id])) {
+        continue;

Comment should start with a capital letter, (and a space between the //), and end with a period.

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
1.92 KB

Updated the comments where needed.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

This fixes a fatal error that happens under certain conditions. The D7 version of the patch has been RTBC for months, and used on many sites I know of, and probably many more.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Some minor nits.

  1. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
    @@ -61,6 +61,11 @@ public function query() {
    +      // Overwitten fields can lead to fields missing from a display that are still
    +      // set in the non-overwritten combined filter.
    

    The comment should be wrapped to 80 characters.

  2. +++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
    @@ -82,6 +82,64 @@ public function testFilterCombineContains() {
    +   * Tests the combine filter handler when a field overwrite is done and fields set in the
    +   * combine filter are removed from the display but not from the combine filter settings.
    

    The first line of a docblock for a method should be on one line.

  3. +++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
    @@ -82,6 +82,64 @@ public function testFilterCombineContains() {
    +          // Add a dummy field to the combined fields to simulate the removed field.
    

    Comment should be wrapped at 80 characters.

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
2.96 KB

So something like this?

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I think that addresses the concerns in #4.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I can confirm that the test fails without the fix. It's good practice to upload a test only patch for bug fixes.

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -61,6 +61,11 @@ public function query() {
+      // Overwitten fields can lead to fields missing from a display that are
+      // still set in the non-overwritten combined filter.

+++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
@@ -82,6 +82,68 @@ public function testFilterCombineContains() {
+   * Tests the combine filter handler when a field overwrite is done

I think you mean overridden/override here.

Also I'm not sure about the fix. I think the just ignoring a removed field might end up exposing data you don't want to. But fixing this will mean using configuration dependencies on config entity delete which feels like a huge thing. I guess we should log a warning if this is occurring.

alexpott’s picture

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -61,6 +61,11 @@ public function query() {
+      // Overwitten fields can lead to fields missing from a display that are
+      // still set in the non-overwritten combined filter.

+++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
@@ -82,6 +82,68 @@ public function testFilterCombineContains() {
+   * Tests if the filter can handle removed fields.
...
+   * Tests the combine filter handler when a field overwrite is done
...
+          // Add a dummy field to the combined fields to simulate
+          // the removed field.

Actually you mean deleted...

Lendude’s picture

I did add a patch with just a failing test in the original post, but hid it, and it turns out it never gets tested by the testbot then, live and learn.

Multiple scenarios that I can think of make this error occur:

  1. Add fields to the combined field filter, then remove a field from that display -> bug. Here giving some warning that the config is not ok and you need to fix it, makes sense
  2. Add fields to the combined field filter, make a second display, override the field settings for this display and remove a field -> bug. Here your settings in the non-overridden filter settings are ok for display 1 and not for display 2. So what now? You need to override your filter settings for it to work? That's pretty annoying if you have a large number of displays where the filter settings are the same.
  3. The original D7 issue report was due to fields not being available to certain users because of field access modules. I have no idea if this available in D8 yet, but I imagine it will be. No idea how you could handle this, because this could be different on a per user basis.
  4. Deleting a field from an entity that is used in the filter -> bug when searching. The Views UI gives a broken/missing handler, but it still tries to add the field to the filter and gives this bug. This actaully gives some feedback, but still causes a fatal error.

So when I say overridden (or my typoed version of that), I mean all these things. I just can't think of a term that covers all that.

The current 'fix' handles all these scenarios. But I agree, it's not a 'fix' as so much as it is an 'ignore'. I can't really think of a good option that handles all these scenarios, but I'm not a D8 Views expert in any way, so if other people have ideas, great!

jhedstrom’s picture

jhedstrom’s picture

jhedstrom’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: d8_call_to_ensure_my_table.test-only-SHOULD-FAIL.patch, failed testing.

jhedstrom’s picture

Also I'm not sure about the fix. I think the just ignoring a removed field might end up exposing data you don't want to. But fixing this will mean using configuration dependencies on config entity delete which feels like a huge thing. I guess we should log a warning if this is occurring.

The field object has been removed from the display, so I don't think it will expose data. This fix simply prevents the non-overridden combined filter from attempting to call methods on a removed overridden field. I don't think it's an error that would warrant logging, since it might actually be a desired configuration.

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.94 KB
2.97 KB

I've updated the issue summary with the scenarios I outlined.

Also updated the comments in the patch as per #7 and #8.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I think this is RTBC. I also think it is the appropriate fix, since the logic skips over fields that are specified in the combined filter, but have been removed by the current display.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: d8_call_to_ensure_my_table-2370251-15.patch, failed testing.

Status: Needs work » Needs review
jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC now that the bot is happy.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The point about exposing data is that you are expecting the resulting list to be filtered by some value but now it is now. In fact I think the security aspect of this might mean the solution is wrong. If a filter is broken I think we should be displaying nothing.

Lendude’s picture

@alexpott ok I see your point now.

Your scenario would be to use the combined filter to (for example) not display all node that have 'hide' in either the title or the body field. And with the current 'fix', when you remove the title field from the display, you will suddenly start showing nodes with 'hide' in the title, even though it is still set in the filter. That would be bad.

In the scenario's I was thinking about, the combined filter was exposed and used more like a full text search over multiple fields. So with the current 'fix' you would just search on one less field, so in a sense it would 'hide less' instead of 'show more'. That's not so bad.

I for one don't really see a solution where we can facilitate both these scenario's.

Adding a patch that invalidates a display if the combined field filter contains fields not set in the field settings. Is that the direction you were thinking of?
If that is the direction we want to take this, the test needs to be rewritten and I have no idea how to force 'If a filter is broken I think we should be displaying nothing.'.

Adding the patch more as a base for discussion, it needs more work if we want to take this in the direction set in #20.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.87 KB

Talked to @dawehner about this on IRC and he agreed that it was safer to make sure nothing was displayed if somehow fields are missing when redering the view. And that adding validate() to the filter handler was also a good idea.

So running with that.

New patch, new test. This test only tests if the build has been marked as 'failed' if fields are missing. That feels about right, because in effect the build did fail. Marking it as failed would normally lead to a 404 result, so nothing would be shown.

Lendude’s picture

FileSize
3.6 KB

And this should make sure no fatal errors occur and nothing is shown.

The last submitted patch, 22: 2370251-22-SHOULDFAIL.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -87,6 +95,20 @@ public function query() {
+  public function validate() {

Let's quickly put @inheritdoc on here.

Lendude’s picture

FileSize
3.64 KB

ok, done.

dawehner’s picture

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -86,6 +94,23 @@ public function query() {
+  ¶

Meh, some whitespace, sorry :( :( :(

Lendude’s picture

FileSize
3.64 KB

duh, gone.

uhh or not..

Lendude’s picture

FileSize
3.63 KB

now it's really gone.....I hope...

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Ha

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

This is looking really good now. Just one thing we have to do before we can commit.

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -87,6 +95,23 @@ public function query() {
+  public function validate() {
+    $errors = parent::validate();
+    $fields = $this->view->display_handler->getHandlers('field');
+    foreach ($this->options['fields'] as $id) {
+      if (!isset($fields[$id])) {
+        // Combined field filter only works with fields that are in the field
+        // settings.
+        $errors[] = $this->t('Field %field set in %filter is not set in this display.', array('%field' => $id, '%filter' => $this->adminLabel()));
+        break;
+      }
+    }
+    return $errors;

Is it possible to add test coverage of this validation?

Lendude’s picture

FileSize
680 bytes
3.74 KB

Something like this?

$view->validate() should return a empty array when successful, just check that in this case it doesn't.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -87,6 +95,23 @@ public function query() {
+        $errors[] = $this->t('Field %field set in %filter is not set in this display.', array('%field' => $id, '%filter' => $this->adminLabel()));

+++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
@@ -82,6 +82,53 @@ public function testFilterCombineContains() {
+    $this->assertNotIdentical(array(), $view->validate());

Let's check that it contains the expected error

Lendude’s picture

Ok, so something like this?

Lendude’s picture

Status: Needs work » Needs review
FileSize
3.92 KB

back to needs 'needs review'

alexpott’s picture

Status: Needs review » Needs work

@Lendude

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -87,6 +95,23 @@ public function query() {
+        $errors[] = $this->t('Field %field set in %filter is not set in this display.', array('%field' => $id, '%filter' => $this->adminLabel()));

+++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
@@ -82,6 +82,54 @@ public function testFilterCombineContains() {
+    $this->assertTrue(in_array('Field <em class="placeholder">dummy</em> set in <em class="placeholder">Global: Combine fields filter</em> is not set in this display.', $errors['default']));

It'd be best to change the test to use the t() function. Also changing it to do this is better because the assertion messages with be better:

$this->assertEqual(reset($errors), t('Field %field set in %filter is not set in this display.', array('%field' => 'dummy', '%filter' => 'Global: Combine fields filter')));

Then if they are not equal we'll get a super helpful error message.

Lendude’s picture

Status: Needs work » Needs review
FileSize
3.92 KB

Nice.

Changed as per #36

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The feedback from alex seemed to be addressed now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed db72d22 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed db72d22 on 8.0.x
    Issue #2370251 by Lendude, jhedstrom: Removed fields in Views Combined...

Status: Fixed » Closed (fixed)

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