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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

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

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

tim.plunkett’s picture

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

Status: Needs work » Needs review
FileSize
5.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

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

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