hook_views_pre_render is broken if your theme implements a base theme [#1972768]

We introduced a bug in #1895266: Convert views to use the ModuleHandler

// Let the themes play too, because pre render is a very themey thing.
       foreach ($GLOBALS['base_theme_info'] as $base) {
-        $function = $base->name . '_views_pre_render';
-        if (function_exists($function)) {
-          $function($this);
-        }
-      }
-      $function = $GLOBALS['theme'] . '_views_pre_render';
-      if (function_exists($function)) {
-        $function($this);
+        $module_handler->invoke($base, 'views_pre_render', array($this));
       }

     // Let the themes play too, because post render is a very themey thing.
     foreach ($GLOBALS['base_theme_info'] as $base) {
-      $function = $base->name . '_views_post_render';
-      if (function_exists($function)) {
-        $function($this);
-      }
-    }
-    $function = $GLOBALS['theme'] . '_views_post_render';
-    if (function_exists($function)) {
-      $function($this, $this->display_handler->output, $cache);
+      $module_handler->invoke($base, 'views_post_render', array($this));
     }

Both hunks assume the ModuleHandler::invoke can accept an object as the first argument but it can't... the original code that did $base->name was correct.

  /**
   * Invokes a hook in a particular module.
   *
   * @param string $module
   *   The name of the module (without the .module extension).
   * @param string $hook
   *   The name of the hook to invoke.
   * @param ...
   *   Arguments to pass to the hook implementation.
   *
   * @return mixed
   *   The return value of the hook implementation.
   */
  public function invoke($module, $hook, $args = array());

Comment	File	Size	Author
#10	1972768-10.patch	2.4 KB	patrickd
#10
#10	1972768-10-fail.patch	1.25 KB	patrickd
#10
#10	1972768-10-interdiff.txt	3.6 KB	patrickd
#9	1972768-9.patch	1.86 KB	damiankloip
#9
#1	1972768.views-with-base-theme.1.patch	1.15 KB	alexpott
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

File	Size
1972768.views-with-base-theme.1.patch	1.15 KB

Comment #2

xjm

she/her

English

CreditAttribution: xjm commented 17 April 2013 at 14:09

Issue tags:

+Needs tests

Sweet, thanks @alexpott!

Comment #3

17 April 2013 at 15:04

Status:

Needs review

» Needs work

The last submitted patch, 1972768.views-with-base-theme.1.patch, failed testing.

Comment #4

dawehner

German

CreditAttribution: dawehner commented 22 April 2013 at 21:21

Issue tags:

+VDC

Missing tag

Comment #5

patrickd CreditAttribution: patrickd commented 7 May 2013 at 15:36

Status:

Needs work

» Needs review

#1: 1972768.views-with-base-theme.1.patch queued for re-testing.

Comment #6

patrickd CreditAttribution: patrickd commented 7 May 2013 at 17:02

Status:

Needs review

» Reviewed & tested by the community

simple bug, simple patch, lets get this in

Comment #7

patrickd CreditAttribution: patrickd commented 7 May 2013 at 17:04

Status:

Reviewed & tested by the community

» Needs work

awww.. needs tests

wondering what to test here though?

Comment #8

damiankloip CreditAttribution: damiankloip commented 7 May 2013 at 18:57

Assigned:

Unassigned

» damiankloip

I think I know what we can do here...

Comment #9

damiankloip CreditAttribution: damiankloip commented 20 May 2013 at 22:36

Status:

Needs work

» Needs review

File	Size
1972768-9.patch	1.86 KB

So, Maybe we could do something like this? Not sure if it's working.

Comment #10

patrickd CreditAttribution: patrickd commented 20 May 2013 at 23:35

File	Size
1972768-10-interdiff.txt	3.6 KB
1972768-10-fail.patch	1.25 KB

1972768-10.patch	2.4 KB

Some small errors, but basically it was the right direction
- The actual fix of the issue was missing (patch #1)
- The hooks were already implemented in the views_test_data module (but in views_test_data.views_execution.inc)

Comment #11

damiankloip CreditAttribution: damiankloip commented 20 May 2013 at 23:51

Yeha, sorry. That was a really rough patch to outline the method.

Nice, they look like good improvements to me, might as well re use the pre and post render hook implementations from views_test_data.

Comment #12

patrickd CreditAttribution: patrickd commented 20 May 2013 at 23:53

No probs, it was a good outline.

1972768-10.patch is already re-using these implementations!

Comment #13

dawehner

German

CreditAttribution: dawehner commented 21 May 2013 at 04:45

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsHooksTest.phpundefined
@@ -94,6 +94,20 @@ public function testHooks() {
+    $this->container->get('state')->set('views_hook_test_views_pre_render', FALSE);
+    $this->container->get('state')->set('views_hook_test_views_post_render', FALSE);
...
+    $this->assertTrue($this->container->get('state')->get('views_hook_test_views_pre_render'), 'The hook_views_pre_render was invoked in the base theme.');
+    $this->assertTrue($this->container->get('state')->get('views_hook_test_views_post_render'), 'The hook_views_post_render was invoked in the base theme.');

Just in case you want to rerole ... we could store $this->container->get('state') in a variable so that would be easier to read.

Comment #14

patrickd CreditAttribution: patrickd commented 22 May 2013 at 05:23

Status:

Needs review

» Needs work

oops, well when a patch is supposed to fail, for sure it passes XD

Turns out that using the existing hook implementations from views_test_data aren't a very good idea, because they always get called on views->render(), so to test whether they were called as "base theme", we have to count how often they've been called.
That kind of sucks and states are never good for testing..

someone spontaneously has a better approach to test this?

Comment #15

damiankloip CreditAttribution: damiankloip commented 22 May 2013 at 17:06

Assigned:

damiankloip

» Unassigned

Yeah, it's a tricky thing, because we are dealing with good ol' fashioned hooks we can't mock anything :/

Unfortunately, I think we might need to go with a mixture of your last patch, and my initial patch (using different hooks that are called for the base theme).