This one is slightly complex, but I'll try to explain simply...

Suppose one has a theme function/file with a preprocessor in an include file, the include file will be added by Drupal to the theme registry, and when calling the theme function the preprocessor will be called also. If however, a suggestion is used, with the theme function as a base, then the include will not be included, even though the preprocessor functions for the base function will be called by Drupal (though they aren't around to be called, so silently fail.)

Marking as needs review to see if the testbot picks up the failing test (which tests this issue.)

CommentFileSizeAuthor
#9 drupal-theme-preprocess-include.patch5.17 KBsteven jones
#6 hook_suggest_inc_1096770_6.patch1.93 KBdvessel
#2 drupal-theme-preprocess-include-fix.patch4.33 KBsteven jones
#1 drupal-theme-preprocess-include.patch3.67 KBsteven jones
drupal-theme-preprocess-include.patch3.69 KBsteven jones

Comments

steven jones’s picture

steven jones commented
StatusFileSize
new drupal-theme-preprocess-include.patch3.67 KB

Actually that test was broken.

steven jones’s picture

steven jones commented
StatusFileSize
new drupal-theme-preprocess-include-fix.patch4.33 KB

And a possible fix?

dvessel’s picture

dvessel commented
Status: Needs review » Needs work

This should be done while building the registry. Including it from theme() is not a good idea. We should work towards simplifying theme().

steven jones’s picture

steven jones commented
Status: Needs work » Needs review

Either way, theme will need to include the extra files right?

Can you justify your statement:

Including it from theme() is not a good idea.

Please?

dvessel’s picture

dvessel commented
Status: Needs review » Needs work

That wasn't helpful was it? I meant to say that the include should be happening from the existing include statement. We shouldn't be adding another.

The best place to process it is here I think:

http://api.drupal.org/api/drupal/includes--theme.inc/function/drupal_fin...
http://api.drupal.org/api/drupal/includes--theme.inc/function/drupal_fin...

dvessel’s picture

dvessel commented
Status: Needs work » Needs review
StatusFileSize
new hook_suggest_inc_1096770_6.patch1.93 KB
dvessel’s picture

dvessel commented

I'm completely behind on writing functional tests so if you could re-roll that with the test, I think this will be ready to go in.

steven jones’s picture

steven jones commented
Status: Needs review » Needs work

Needs work then...

steven jones’s picture

steven jones commented
Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
StatusFileSize
new drupal-theme-preprocess-include.patch5.17 KB

Added the test from comment 1.

Changing to Drupal 8.

steven jones’s picture

steven jones commented

#1: drupal-theme-preprocess-include.patch queued for re-testing.

I want to make sure that the test fails in D8

Status: Needs review » Needs work

The last submitted patch, drupal-theme-preprocess-include.patch, failed testing.

dvessel’s picture

dvessel commented

It failed. What's next?

steven jones’s picture

steven jones commented

Well, the test correctly shows that your 'fix' to the issue doesn't work, so I'd guess you'd want to fix that...

dvessel’s picture

dvessel commented

It was originally patched for 7.

I want to make sure that the test fails in D8

It failed.

From the test results:

NOTICE: Undefined index: theme_test_preprocess_indentation

From a bad test.

steven jones’s picture

steven jones commented

?

The test seems to have correctly detected that the fix did not fix the issue, hence the fail? Has the theme system changed massively already? Maybe you could modify the test to make it work with your code, as you must have a test for your fix of some kind that passes?

steven jones’s picture

steven jones commented
Status: Needs work » Needs review

#2: drupal-theme-preprocess-include-fix.patch queued for re-testing.

dvessel’s picture

dvessel commented

Okay, looks like it didn't fix it. That section in theme() where it switches hooks by looking for the base is flawed. I'd hate to pile more workarounds because of it but there isn't much choice until it's fixed.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Needs work
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 8.0.x-dev » 7.x-dev
Issue summary: View changes
Status: Needs work » Active

In Drupal 8 \Drupal\system\Tests\Theme\ThemeSuggestionsAlterTest::testSuggestionsAlterInclude() proves this is working. Moving back to Drupal 7.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Active » Needs work

Back to "needs work" as there's an existing patch that just needs to be improved upon.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.