Problem/Motivation

A view that uses an exposed filter on a page can be accessed from a link such as http://d.8/new?since=2014-09-10 and the value of since will be used, filtering the view. A REST Export view with the same configuration will not be filtered by the exposed value. If there is some other mechanism that is supposed to be used to populate the filter, I can't find any documentation of it and it seems that the pattern from a page view should work with a REST view.

Repro steps:

  1. Install head under Apache/2.4.9 (Unix) PHP/5.5.15.
  2. Create two content items with different content dates.
  3. Create REST Export view with exposed filter greater than "Authored on" and set filter identifier.
  4. Make REST request to view endpoint, using filter identifier and a value in URL that should only display one node. Response will incorrectly include both nodes.
  5. Clone view to be a Page and use the same URL. Response will be correct.

Proposed resolution

Umm....

Remaining tasks

Identify cause & patch?

User interface changes

Existing feature will work as expected.

API changes

None?

Files: 
CommentFileSizeAuthor
#9 interdiff-2340623-7-9.txt797 bytesjohnennew
#9 2340623-9.patch8.09 KBjohnennew
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,925 pass(es). View
#7 allow_rest_exposed_filter-2340623-7.patch7.84 KBjohnennew
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,027 pass(es). View
#1 allow-rest-exposed-2340623-1.patch453 bytesR.Muilwijk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,414 pass(es). View

Comments

R.Muilwijk’s picture

Status: Active » Needs review
FileSize
453 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,414 pass(es). View

Using the exposed filters is fairly easy and could be a valuable addition to a GET REST endpoint. Patch attached.

sebas5384’s picture

Hey @R.Muilwijk thanks!! you saved my day ;)

This worked exactly like I was expecting to work, I don't know if I'm the right person to review this because I don't know what kind of problems could bring this feature.
But it worked!, and for curious I would love to know why this wasn't released at first.

Please!! somebody from the core, this is a really important feature, not just for filters, for example exposed pagination.

Cheers!

klausi’s picture

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

Makes sense, can we get a test case for this?

kalabro’s picture

Assigned: Unassigned » kalabro

I'm working on this at DrupalCamp Baltics 2015 Code Sprint!

kalabro’s picture

Assigned: kalabro » Unassigned

@klausi Could you please explain what kind of test would you like to see here? As I can see, REST module isn't responsible for Views logic tests. Views exposed filters are tested in Views module itself.

klausi’s picture

We should write a web test that does something similar as described in the issue summary. Create a REST view, add some nodes, sent a request with an exposed filter, make sure you only get filtered results back.

johnennew’s picture

Status: Needs work » Needs review
FileSize
7.84 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,027 pass(es). View

Please find a new patch with a test attached.

1. Adds a new test view with an exposed title filter set to filter by titles which start with the provided value
2. Creates 3 nodes specifying the title
3. Queries without the title query parameter to check all 3 nodes are returned - this shows the patch is not changing the no query parameter behaviour
4. Queries again with a title query parameter which should reduce the number of nodes returned to be 2 - this shows providing a parameter applies the query and filters the results.

dawehner’s picture

Status: Needs review » Needs work

Thank you for adding a test!

diff --git a/core/modules/rest/src/Plugin/views/display/RestExport.php b/core/modules/rest/src/Plugin/views/display/RestExport.php
index 2d3a5b0..857faac 100644

index 2d3a5b0..857faac 100644
--- a/core/modules/rest/src/Plugin/views/display/RestExport.php

--- a/core/modules/rest/src/Plugin/views/display/RestExport.php
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -151,7 +151,7 @@ protected function getType() {

@@ -151,7 +151,7 @@ protected function getType() {
    * {@inheritdoc}
    */
   public function usesExposed() {
-    return FALSE;
+    return TRUE;
   }
 

@@ -610,4 +610,54 @@ public function testGroupRows() {
+    $this->assertEqual($result, $expected, 'Querying a view with no exposed filter returns all nodes.');
+
+    // Test that title starts with 'Node 11' query finds 2 of the 3 nodes.
+    $result = $this->drupalGetJSON('test/serialize/node-exposed-filter', ['query' => ['title' => 'Node 11']]);
+
+    $expected = array(
+      0 => array(
+        'nid' => $node1->id(),

I think we should better test the cacheabilty metadata introduced by those exposed filters as well, just to be sure. So use \Drupal\system\Tests\Cache\AssertPageCacheContextsAndTagsTrait and check assertCacheContexts() that it includes the proper query parameters

johnennew’s picture

Status: Needs work » Needs review
FileSize
8.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,925 pass(es). View
797 bytes

Hi @dawehner - I've added a test which makes use of assertCacheContexts(). I couldn't see if it was possible to check if the specific query parameters were being included in the cache - is this possible? I'm at DrupalCon Barcelona today (Friday 25th) if you want to ping me any instructions Drupal Contribute as ceng_

vincic’s picture

Any chance to get this into final Drupal 8.0?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +rc target triage

Wow nice, I'd not have expected that its sad easy. Nice work

xjm’s picture

Title: Filters exposed in views for REST Export don't use values from URL » Views REST export does not support exposed filters
Issue tags: -rc target triage +rc target

Retitling to clarify the bug. @effulgentsia and I agreed that this can be fixed during RC because this is a common usecase, the fix is non-disruptive, and we don't actually need to change anything except enabling the functionality.

I do find myself wondering why this got set to false in the first place, though. I might ask @damiankloip or check the git history. Leaving at RTBC but I'd want to double-check that before committing this patch myself--someone else can still commit it though if they want.

dawehner’s picture

Well, I just think we didn't believed that it actually works that easy.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

@phenaproxima checked and it's been set to FALSE since comment #9 of #1819760-9: Add a REST export display plugin and serializer integration., which was basically the first patch in that issue based on the display plugin refactor. It's not discussed on the issue at all there, so indeed it was not disabled for any specific reason other than what @dawehner suggests in #13 or scope management or whatnot.

Committed and pushed to 8.0.x. Thanks!

  • xjm committed 1f4013c on 8.0.x
    Issue #2340623 by ceng, R.Muilwijk, dawehner, klausi, Rj-dupe-1, xjm,...
damiankloip’s picture

Yes. I think we just had it disabled....because. I wouldn't be surprised if there are a few issues, and expectations based on a regular exposed form. It works so easily because views exposed filters/form uses GET anyway.

Over all +1. Just don't blame me if some things don't work :)

  • xjm committed 1f4013c on 8.1.x
    Issue #2340623 by ceng, R.Muilwijk, dawehner, klausi, Rj-dupe-1, xjm,...

Status: Fixed » Closed (fixed)

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