Was noticing an excessive number of calls to drupal_add_css() and discovered that the logic in views_plugin_cache::gather_headers() is a bit broken. The array it's checking against to get the diffs is cleared right before it, which results in the entire $css and $js array being cached.

Attached patch simplifies the logic and appears to work well in my testing.

CommentFileSizeAuthor
#4 1530740-cache-storage.patch5.77 KBdawehner
#2 1530740-test-cache-storage.patch3.47 KBdawehner
#2 1530740-cache-storage.patch5.18 KBdawehner
views_fix_cssjs_cache_add.patch1.71 KBmsonnabaum

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Active » Needs review

Applies cleanly and seems to reduce the time spent in drupal_add_css in brief xhprof testing.

Let's see what testbot thinks of it.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 1530740-cache-storage.patch5.18 KB
new 1530740-test-cache-storage.patch3.47 KB

Here is a test for this issue. There is one expected test fail.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work

Overall this looks great, and it's wonderful to see a test!
Coding/grammar nitpicks:

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // @FIXME: The cache plugin expects settings['data'] to be set

Use @todo, and no :, and a trailing full stop. Unless this is to be fixed before committing?

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // Take sure that this css is not added when running the cached view.

s/Take sure/Make sure

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    $this->assertFalse(isset($css[$system_css_path]), 'Take sure that unrelated css is not added.');
+    $this->assertFalse(isset($js[$system_js_path]), 'Take sure that unrelated js is not added.');

s/Take sure/Make sure

+++ b/plugins/views_plugin_cache.incundefined
@@ -201,32 +201,18 @@ class views_plugin_cache extends views_plugin {
+    // Get javascript after/before views renders

Missing trailing full stop.

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // @FIXME: The cache plugin expects settings['data'] to be set

Use @todo, and no :, and a trailing full stop. Unless this is to be fixed before committing?

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // Take sure that this css is not added when running the cached view.

s/Take sure/Make sure

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    $this->assertFalse(isset($css[$system_css_path]), 'Take sure that unrelated css is not added.');
+    $this->assertFalse(isset($js[$system_js_path]), 'Take sure that unrelated js is not added.');

s/Take sure/Make sure

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 1530740-cache-storage.patch5.77 KB

dawehner-- for doing the same stupid thing over and over again.

I'm confused you posted the same review twice, how does dreditor allow you to do that.

dawehner’s picture

dawehner
Primary language German
commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

Let's get it in, the tests pass and the code itself looks fine.

This maybe is worth to backport.

chris matthews’s picture

chris matthews commented
Issue summary: View changes
Status: Patch (to be ported) » Closed (outdated)

The Drupal 6 branch is no longer supported, please check with the D6LTS project if you need further support. For more information as to why this issue was closed, please see issue #3030347: Plan to clean process issue queue