Attached patch fixes a couple bugs.

The views UI uses Database, but doesn't "use" it. This bug isn't ever found because we're checking for an instance of views_plugin_query_default, which is no longer the right class name. Both of these result in the query args not being replaced in the preview.

CommentFileSizeAuthor
#11 vdc-1820332-11.patch2.67 KBtim.plunkett
#11 interdiff.txt949 bytestim.plunkett
#9 drupal-1820332-7.patch2.66 KBdawehner
#4 core-1820332-4.patch2.68 KBdawehner
#4 core-1820332-4-test.patch1.74 KBdawehner
views_ui_db_class_bug.patch963 bytesmsonnabaum

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review
Issue tags: +Needs tests
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +VDC
aspilicious’s picture

aspilicious commented

I have a feeling this needs tests...

dawehner’s picture

dawehner
Primary language German
commented
Priority: Major » Normal
Issue tags: -Needs tests
StatusFileSize
new core-1820332-4-test.patch1.74 KB
new core-1820332-4.patch2.68 KB

We need tests for so many things.

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

The last submitted patch, core-1820332-4-test.patch, failed testing.

dawehner’s picture

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

Proven that there is a bug.

damiankloip’s picture

damiankloip commented
Status: Needs review » Needs work

Looks pretty good.

I will create a follow up issue for testing all views settings in the UI match the set variables.

+++ b/core/modules/views/lib/Drupal/views/Tests/UI/SettingsTest.phpundefined
@@ -91,6 +91,29 @@ function testEditUI() {
+    $this->assertFalse(strpos($xpath[0], 'db_condition_placeholder_0') !== FALSE, 'The placeholders in the views sql are not shown directly..');

We should just test for all placeholders here, not just the first?

Also, should this live in it's own method, like testUIPreview or something?

damiankloip’s picture

damiankloip commented

Also, should this live in it's own method, like testUIPreview or something?

After looking the the testEditUI method, this seems to fit in there, as it tests most other parts of the edit form too.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-1820332-7.patch2.66 KB

Later we should enable the settings via config() and test the output, and just test that the settings form is saving the right config.

aspilicious’s picture

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

This is looking good.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new interdiff.txt949 bytes
new vdc-1820332-11.patch2.67 KB

We use instanceof most everywhere else, let's do that here as well.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Db class name bug in views preview » Views preview uses the wrong query class name
webchick’s picture

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

Makes sense, and comes with tests. Yay!

Committed and pushed to 8.x. Thanks!

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