Preprocess functions in an include file fail to get called when the theme implements a suggestion override [#1430300]

Follow-up from #241570-68: Theme preprocess functions do not get retained when using patterns.

hook_theme() allows for specifying a 'file' key, so that a default theme function and preprocess functions can reside in a separate include file. For example, user_theme() does this for 'user_profile', allowing template_preprocess_user_profile() to reside in user.pages.inc.

We also allow for theme hook suggestions to be specified as a #theme value. For example, a module or theme can implement hook_user_view_alter(), and change $build['#theme'] from 'user_profile' to 'user_profile__' . $account->uid. Doing so would, for example, allow a theme to implement a user-profile--1.tpl.php in order to customize the display of the root user's profile.

In #241570: Theme preprocess functions do not get retained when using patterns, we made sure that the preprocess functions for the base hook (e.g., 'user_profile') are still called, even when the theme implements a specific suggestion override (e.g., user-profile--1.tpl.php).

However, there is currently a bug in theme() where the file in which those base preprocess functions are located isn't included, thereby, the preprocess functions fail to get called unless that file happened to be included elsewhere.

#4 contains a test that demonstrates this by changing the original test from #241570: Theme preprocess functions do not get retained when using patterns from testing 'breadcrumb' to testing a new test theme hook.

Comment	File	Size	Author
#20	1430300-20.patch	5.33 KB	NROTC_Webmaster
#20
#16	1430300-16.patch	4.75 KB	no_commit_credit
#16
#16	interdiff.txt	992 bytes	no_commit_credit
#13	1430300-10.patch	4.75 KB	no_commit_credit
#13
#13	interdiff.txt	1.13 KB	no_commit_credit
#4	theme_base_hook_includes-1430300-4-test-only.patch	3.97 KB	effulgentsia
#4
#4	theme_base_hook_includes-1430300-4.patch	4.76 KB	effulgentsia
#4
#3	1430300-by-mradcliffe-backport-base-hook-include-d7.patch	807 bytes	mradcliffe
#3
#2	1430300-by-mradcliffe-backport-base-hook-include-d7.patch	807 bytes	mradcliffe
#2
#1	0001-Patch-1430300-by-mradcliffe.-Include-files-for-a-bas.patch	1.2 KB	mradcliffe
#1
#1	0001-1430300-by-mradcliffe.-Include-files-for-a-base-hook-d7.patch	1.2 KB	mradcliffe
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 6 February 2012 at 20:43

Status:	Active	» Needs review
Issue tags:		+Needs tests

File	Size
0001-1430300-by-mradcliffe.-Include-files-for-a-base-hook-d7.patch	1.2 KB

0001-Patch-1430300-by-mradcliffe.-Include-files-for-a-bas.patch	1.2 KB

This works for me in D7. I don't have a D8 install to test on.

Comment #2

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 8 February 2012 at 19:09

File	Size
1430300-by-mradcliffe-backport-base-hook-include-d7.patch	807 bytes

Oh, dangit. I didn't format patch on the right branch :(

Here's just a standard git diff patch for d7.

Comment #3

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 8 February 2012 at 19:14

File	Size
1430300-by-mradcliffe-backport-base-hook-include-d7.patch	807 bytes

Nope. I am an idiot sometimes. Sorry for spam.

Comment #4

effulgentsia CreditAttribution: effulgentsia commented 23 February 2012 at 02:45

Issue tags:

-Needs tests

File	Size
theme_base_hook_includes-1430300-4.patch	4.76 KB

theme_base_hook_includes-1430300-4-test-only.patch	3.97 KB

Thanks for the find and fix! Here's a test-only patch that demonstrates the bug, and a patch with that test plus the fix in #1 with a minor code comment tweak. Adding the backport tag so we remember to commit this to 7.x too, though the actual backport work might just be to remove 'core/' from the patch file.

Comment #5

effulgentsia CreditAttribution: effulgentsia commented 23 February 2012 at 02:51

Title:

Template preprocess function base hook includes not included in theme().

» Preprocess functions in an include file fail to get called when the theme implements a suggestion override

Comment #5.0

effulgentsia CreditAttribution: effulgentsia commented 23 February 2012 at 19:38

Issue summary:

View changes

Updated issue summary.

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 23 February 2012 at 22:13

Status:

Needs review

» Reviewed & tested by the community

Thanks, looks good to me.

Comment #7

Lars Toomre CreditAttribution: Lars Toomre commented 24 February 2012 at 02:01

Status:

Reviewed & tested by the community

» Needs work

+++ b/core/modules/simpletest/tests/theme.testundefined
@@ -52,11 +52,19 @@ class ThemeUnitTest extends DrupalWebTestCase {
   /**
-  * Preprocess functions for the base hook should run even for suggestion implementations.
-  */
+   * Ensure preprocess functions for the base hook run even for suggestion implementations.

This documentation block needs work.

The first line of a docblock should start with an active verb tense and the whole line should be less than 80 characters. If necessary include more in an additional comment (after a blank line).

Comment #8

sun

German

Karlsruhe

CreditAttribution: sun commented 24 February 2012 at 02:05

Status:

Needs work

» Reviewed & tested by the community

As of now, that doesn't really apply to test methods in test cases. Before we apply the phpDoc summary there, I'd like to see the existing phpDoc summaries getting fixed.

Comment #9

xjm

she/her

English

CreditAttribution: xjm commented 24 February 2012 at 16:03

Well, I wasn't assertive enough to NW a patch sun reviewed over a docs nit, but I did think the same thing and considered rerolling it before an incident distracted me yesterday. :) And having it over 80 characters is already inconsistent with all our docblocks everywhere.

Here is the existing issue to clean up the docblocks in this file:
#1431664: Clean up API docs for system/tests, J-Z, including subdirectories

The more correct we can make all patches currently going in, the less the cleanup patches will be reroll-inducing, baby-eating nightmares. So I'd disagree with postponing writing proper docs to cleanup issues.

Comment #10

xjm

she/her

English

CreditAttribution: xjm commented 24 February 2012 at 16:04

Assigned:

Unassigned

» xjm

Oh, I'll just do it. :)

Comment #11

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 24 February 2012 at 16:14

Thank you for the tests, effulgentsia. Those look good to me as well.

Comment #12

Lars Toomre CreditAttribution: Lars Toomre commented 24 February 2012 at 16:14

Thank you @xjm.

The comment in #8 left my nose a bit out of joint. Since when do we ever knowingly allow docblock lines to exceed 80 characters?? I can recall many patch comments where @sun himself would note a docblock line exceeded 80 characters.

Comment #13

no_commit_credit CreditAttribution: no_commit_credit commented 24 February 2012 at 16:20

File	Size
interdiff.txt	1.13 KB
1430300-10.patch	4.75 KB

Comment #14

xjm

she/her

English

CreditAttribution: xjm commented 24 February 2012 at 16:21

~~#11~~ #13 just tweaks the docblocks a little. Trivial nitpicky comment change, so leaving the issue at RTBC. :) Thanks everyone!

Comment #15

xjm

she/her

English

CreditAttribution: xjm commented 24 February 2012 at 18:14

Assigned:

xjm

» Unassigned

Comment #16

no_commit_credit CreditAttribution: no_commit_credit commented 24 February 2012 at 21:58

File	Size
interdiff.txt	992 bytes
1430300-16.patch	4.75 KB

Comment #17

xjm

she/her

English

CreditAttribution: xjm commented 24 February 2012 at 21:59

The above fixes a code style issue webchick spotted and also removes t() from the assertion message. References: http://drupal.org/simpletest-tutorial-drupal7#t and #500866: [META] remove t() from assert message.

Comment #18

catch

he/him

English

CreditAttribution: catch commented 27 February 2012 at 03:27

Status:

Reviewed & tested by the community

» Patch (to be ported)

Thanks!

Committed/pushed to 8.x, moving back to 7.x for backport.

Comment #19

catch

he/him

English

CreditAttribution: catch commented 27 February 2012 at 03:28

Version:

8.x-dev

» 7.x-dev

Comment #20

NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented 27 February 2012 at 16:08

Status:

Patch (to be ported)

» Needs review

File	Size
1430300-20.patch	5.33 KB

Here is the port

Comment #21

xjm

she/her

English

CreditAttribution: xjm commented 27 February 2012 at 17:03

Status:

Needs review

» Reviewed & tested by the community

Backport checks out.

Comment #22

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 28 February 2012 at 18:23

Status:

Reviewed & tested by the community

» Fixed

I initially got confused here about the fact that the theme.inc hunk references 'includes' but the test function adds 'file', however it looks like this is just the theme system being inconsistent with itself. xjm pointed out that http://qa.drupal.org/pifr/test/229313 shows the test failing without the patch.

Committed and pushed to 7.x. Thanks!