Problem/Motivation

There is a subtle difference in how RestExport::buildResponse and Feed::buildResponse function with regards to the response object. The former instantiates a response after the view is rendered, while the latter injects an empty response object into the view, so it can have headers added as needed.

For views built on top of the RestExport class, it would be nice to have access to the response object during processing, so for instance something like the Content-Disposition header could be added.

Proposed resolution

Refactor the logic in RestExport::buildResponse to inject an empty response into the views processing pipeline.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 2779807-14.patch4.41 KBjhedstrom
#9 2779807-09.patch4.41 KBjhedstrom
#9 2779807-09-TEST-ONLY.patch3.27 KBjhedstrom
#9 interdiff.txt1.29 KBjhedstrom
#5 2779807-05.patch3.96 KBjhedstrom
#5 2779807-05-TEST-ONLY.patch2.81 KBjhedstrom
#2 2779807-02.patch1.15 KBjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Active » Needs review
FileSize
2779807-02.patch1.15 KB

A practical example of using this can be seen here: https://github.com/d8-contrib-modules/views_data_export/pull/13/files#di...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Drupalaton

Nice cleanup!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This change looks testable.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2779807-05-TEST-ONLY.patch2.81 KB
2779807-05.patch3.96 KB

Good call!

Here's a test that ensures the response object is available when building/rendering the view (the test-only patch is the interdiff).

The last submitted patch, 5: 2779807-05-TEST-ONLY.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/rest/tests/src/Kernel/Views/RestExportTest.php
@@ -0,0 +1,61 @@
+class RestExportTest extends ViewsKernelTestBase {

<3

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/rest/tests/src/Kernel/Views/RestExportTest.php
@@ -0,0 +1,61 @@
+    // No custom header should be set yet.
+    $this->assertEquals($header, $response->headers->get('Custom-Header'));

This is confusing. I guess you meant to have a test where reset_test-views_set_header is not set?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.29 KB
2779807-09-TEST-ONLY.patch3.27 KB
2779807-09.patch4.41 KB

Oops. Indeed, I was planning to do a test before/after. This adds that test.

Status: Needs review » Needs work

The last submitted patch, 9: 2779807-09.patch, failed testing.

The last submitted patch, 9: 2779807-09-TEST-ONLY.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review

Re-queued #9 for testing, as those fails looked unrelated.

Status: Needs review » Needs work

The last submitted patch, 9: 2779807-09.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
2779807-14.patch4.41 KB

I still can't see how those errors are related. Rebasing off of latest 8.3.x.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/rest/tests/src/Kernel/Views/RestExportTest.php
@@ -0,0 +1,71 @@
+    /** @var \Drupal\Core\Cache\MemoryBackend $render_cache */
+    $render_cache = $this->container->get('cache_factory')->get('render');
+    $render_cache->deleteAll();

For future references you could also make your life easier and just invalidate the render cache tag, but nevermind :)

  • catch committed 2a43f31 on 8.3.x 
    Issue #2779807 by jhedstrom: Bring RestExport::buildResponse into line...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

  • catch committed 92adca5 on 8.2.x 
    Issue #2779807 by jhedstrom: Bring RestExport::buildResponse into line...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2779807-14.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Fixed

you are drunk testbot

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +VDC

Nice!

Status: Fixed » Closed (fixed)

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