Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
- Add fields to the combined field filter, then remove a field from that display.
- Add fields to the combined field filter, make a second display, override the field settings for this display and remove a field.
- 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.
- 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
Issue category | Bug, because it breaks drupal |
---|---|
Issue priority | Normal, because it is not a often used feature |
Disruption | No disruption at all |
Comment | File | Size | Author |
---|---|---|---|
#37 | 2370251-37.patch | 3.92 KB | Lendude |
#35 | 2370251-34.patch | 3.92 KB | Lendude |
#34 | interdiff-2370251-32-34.txt | 981 bytes | Lendude |
#32 | interdiff-2370251-29-32.txt | 680 bytes | Lendude |
#29 | 2370251-29.patch | 3.63 KB | Lendude |
Comments
Comment #1
jhedstromComment should start with a capital letter, (and a space between the
//
), and end with a period.Comment #2
LendudeUpdated the comments where needed.
Comment #3
jhedstromThis 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.
Comment #4
alexpottSome minor nits.
The comment should be wrapped to 80 characters.
The first line of a docblock for a method should be on one line.
Comment should be wrapped at 80 characters.
Comment #5
LendudeSo something like this?
Comment #6
jhedstromI think that addresses the concerns in #4.
Comment #7
alexpottI can confirm that the test fails without the fix. It's good practice to upload a test only patch for bug fixes.
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.
Comment #8
alexpottActually you mean deleted...
Comment #9
LendudeI 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:
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!
Comment #10
jhedstromComment #11
jhedstromComment #12
jhedstromComment #14
jhedstromThe 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.
Comment #15
LendudeI've updated the issue summary with the scenarios I outlined.
Also updated the comments in the patch as per #7 and #8.
Comment #16
jhedstromI 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.
Comment #19
jhedstromBack to RTBC now that the bot is happy.
Comment #20
alexpottThe 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.
Comment #21
Lendude@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.
Comment #22
LendudeTalked 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.
Comment #23
LendudeAnd this should make sure no fatal errors occur and nothing is shown.
Comment #25
dawehnerLet's quickly put @inheritdoc on here.
Comment #26
Lendudeok, done.
Comment #27
dawehnerMeh, some whitespace, sorry :( :( :(
Comment #28
Lendudeduh, gone.
uhh or not..
Comment #29
Lendudenow it's really gone.....I hope...
Comment #30
dawehnerHa
Comment #31
alexpottThis is looking really good now. Just one thing we have to do before we can commit.
Is it possible to add test coverage of this validation?
Comment #32
LendudeSomething like this?
$view->validate() should return a empty array when successful, just check that in this case it doesn't.
Comment #33
alexpottLet's check that it contains the expected error
Comment #34
LendudeOk, so something like this?
Comment #35
Lendudeback to needs 'needs review'
Comment #36
alexpott@Lendude
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:
Then if they are not equal we'll get a super helpful error message.
Comment #37
LendudeNice.
Changed as per #36
Comment #38
dawehnerThe feedback from alex seemed to be addressed now.
Comment #39
alexpottCommitted db72d22 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.