Views core stores CSS/JS and head contents in cache. But view can send some HTTP-headers, which are missing in cached output. For example, "Content-type" header from RSS feed is missing and output from cache has default HTML content type, which is wrong. Patch just adds HTTP headers to storage and reproduce them on retrieving from cache.

Probably its place is in Views core, not sure. Made from 6.x-2.2. It increases minimal PHP version to 5.0 because headers_list() function was added in PHP 5.0. This shouldn't be a problem nowadays.

CommentFileSizeAuthor
#47 Screen Shot 2022-07-22 at 9.30.13 am.png71.93 KBdarvanen
#29 drupal-1312962-29.patch5.03 KBdawehner
#29 interdiff.txt772 bytesdawehner
#26 drupal-1312962-26.patch5.06 KBolli
#26 interdiff.txt973 bytesolli
#23 drupal-1312962-23.patch4.79 KBolli
#23 interdiff.txt1.06 KBolli
#21 drupal-1312962-21.patch4.79 KBdawehner
#20 drupal-1312962-20.patch3.63 KBdawehner
#17 views-1312962-17.patch4.34 KBdawehner
#11 views-1312962-11.patch5.56 KBDmitriy.trt
#7 views-1312962-7.patch5.37 KBDmitriy.trt
#6 views-1312962-6.patch4.79 KBdawehner
#5 views-1312962-5.patch5.37 KBDmitriy.trt
#3 views-1312962-3.patch4.55 KBDmitriy.trt
#2 views-6.x-2.12-cache-http-headers.patch1.79 KBDmitriy.trt
views_content_cache-6.x-2.2-http-headers.patch1.57 KBDmitriy.trt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hefox’s picture

hefox CreditAttribution: hefox commented
Project: Views content cache » Views (for Drupal 7)
Status: Needs review » Needs work

Yea, views seems better, nay?

Dmitriy.trt’s picture

Dmitriy.trt CreditAttribution: Dmitriy.trt commented
Version: 6.x-2.2 » 6.x-2.12
Category: feature » bug
Status: Needs work » Needs review
FileSize
views-6.x-2.12-cache-http-headers.patch1.79 KB

Here is updated patch based on Views 6.x-2.12. And for Views it is a bug, which also seems to affect 6.x-3.x and 7.x-3.x branches.

Dmitriy.trt’s picture

Dmitriy.trt CreditAttribution: Dmitriy.trt commented
Version: 6.x-2.12 » 7.x-3.x-dev
FileSize
views-1312962-3.patch4.55 KB

OK, I understand that Views for Drupal 6 supports PHP 4 and the above patch won't be committed because of it.

Here is a port to Drupal 7 with the test to check that cached and non-cached RSS feed returns the same content type header.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Thanks for the patch. It is really great that you have added some tests as well.

What about adding a line which checks that the result is coming from the actual cache?

Dmitriy.trt’s picture

Dmitriy.trt CreditAttribution: Dmitriy.trt commented
FileSize
views-1312962-5.patch5.37 KB

Added a few more assertions, including the one that we get the second result from cache.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Version: 7.x-3.x-dev » 8.x-3.x-dev
Status: Needs review » Patch (to be ported)
FileSize
views-1312962-6.patch4.79 KB

Thanks for the tests again, they are simply great!

This is the patch i committed to 7.x-3.x, just had some small improvements.

Dmitriy.trt’s picture

Dmitriy.trt CreditAttribution: Dmitriy.trt commented
Status: Patch (to be ported) » Needs review
FileSize
views-1312962-7.patch5.37 KB

To be honest, I don't familiar with changes coming with Drupal 8, but looks like the cache test case still have the same old location and format. What about a re-roll of the patch from 7.x? And the test will be converted to the new format later together with the other tests inside this test case if necessary.

Please, let me know if I'm really wrong about it.

Status: Needs review » Needs work

The last submitted patch, views-1312962-7.patch, failed testing.

Dmitriy.trt’s picture

Dmitriy.trt CreditAttribution: Dmitriy.trt commented

OK, please forget about my previous comment. I'll port it in my free time on this week.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

This would be really helpful, because you know exactly the problem.

If you don't have time, someone will do it hopefully at some point in the future.

Dmitriy.trt’s picture

Dmitriy.trt CreditAttribution: Dmitriy.trt commented
Status: Needs work » Needs review
FileSize
views-1312962-11.patch5.56 KB

Sorry for a delay.

It should be safe to cache whole Response object if it has been changed on rendering, because it gets initialized in View::render(), right before rendering process. Comparing it field-by-field would be a waste of time here.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Thanks for porting the patch! Please don't worry about the delay as this is already more then i would expect people to do at this point of d8 development.

The response could also contain the output, see http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/Response.html
and http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/ResponseHead...

Maybe it would make sense to produce a diff of the components as well

Dmitriy.trt’s picture

Dmitriy.trt CreditAttribution: Dmitriy.trt commented

Here are possible improvements:

  • Store response headers diff + response properties diff. Problems: we'll loose status text, because there is no way to get it from outside of the Response class except for response text parsing or patching the core.
  • Do the response initialization right after the caching start to be 100% sure we don't need a diff. Of course, we'll make sure the response object is initialized on restoring from cache even if there is no cached response. Possible problem: the exposed form plugin will not be able to change the response object from the pre_render() function. But current implementations don't do it and there is a post_render() function where they still can change the response. Probably it should be documented in views_plugin_exposed_form::pre_render().

What do you think? Which one should we implement?

About the response content, I think we should cache it too if it was set by the rendering code. It is possible to produce double caching of content because of not well written plugins, but it would be even worse if we drop response content on caching: if some component sets the response content, then it expects the content is still there after restoring from cache.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: +VDC
dawehner’s picture

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

As all the files moved this patch have to be rerolled.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

If something is setting the response output to early it's his own fault,
as long you can be sure views itself doesn't do that, i think that's okay.
Actually i don't see a reason why exposed_form::pre_render shouldn't be done a bit later.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
views-1312962-17.patch4.34 KB

This just contains a rerole

Status: Needs review » Needs work

The last submitted patch, views-1312962-17.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
drupal-1312962-20.patch3.63 KB

Let's port this patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
drupal-1312962-21.patch4.79 KB

Missing test file, sorry.

Status: Needs review » Needs work

The last submitted patch, drupal-1312962-21.patch, failed testing.

olli’s picture

olli CreditAttribution: olli commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.06 KB
drupal-1312962-23.patch4.79 KB

Added +s.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Oh good points, thank you!

Status: Needs review » Needs work

The last submitted patch, drupal-1312962-23.patch, failed testing.

olli’s picture

olli CreditAttribution: olli commented
Status: Needs work » Needs review
FileSize
interdiff.txt973 bytes
drupal-1312962-26.patch5.06 KB

The last submitted patch, drupal-1312962-26.patch, failed testing.

olli’s picture

olli CreditAttribution: olli commented
Status: Needs work » Needs review
Issue tags: +VDC

#26: drupal-1312962-26.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt772 bytes
drupal-1312962-29.patch5.03 KB

Rerolled against HEAD, this looks really great, thank you for taking that over!

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Issue tags: +D8 cacheability

Seems like we need drupal_render to start collecting headers in addition to css/js/library?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +WSCCI, +Blocks-Layouts

@moshe

It feels like this should be not done on the render api level but somehow on the SCOTCH level, no idea basically, but it feels wrong to set something which clearly is connected with the request/subrequest onto the rendering level.

Adding some random tags.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

For now, those are the same thing. WHen you are building up your response, we need a way to collect headers/css/js/etc. Today, we often use render arrays for that. One day we won't.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

To be honest it feels entirely wrong to store headers in render API, but I guess it's too late in the release cycle to wait for a proper implementation of scotch?

We could also get this hacky way in, and then convert to scotch, once they have figured it out.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: -WSCCI, -VDC, -Blocks-Layouts, -D8 cacheability

#29: drupal-1312962-29.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1312962-29.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
CreditAttribution: darvanen commented
Issue summary: View changes
Status: Needs work » Closed (outdated)
Issue tags: +Bug Smash Initiative
FileSize
Screen Shot 2022-07-22 at 9.30.13 am.png71.93 KB

This was discusses as part of a group triage session in #bugsmash (slack channel).

Comment from @larowlan:

this is likely outdated now we have Response objects

I manually tested on a fresh installation of 9.5.x with all defaults - i.e. caching turned on.

Went to the provided /rss.xml page and refreshed a few times, Content-Type header remains correct:

Marking as outdated. If you find reason to revive this issue please feel free to do so with clear steps to reproduce and an updated issue summary.