When using a pattern for a theme function (i.e, theme(array('foo__bar', 'foo'), ...);) the preprocess functions aren't being retained of the pattern has been declared.

The easiest way to replicate this is to install Views 2 alpha4. Make sure the frontpage view is enabled. Visit 'frontpage'.

Now, copy views-view-unformatted.tpl.php to your theme directory, and add a piece of text to it to identify our theming function. Clear your cache and reload. This works fine.

Rename this to views-view-unformatted--frontpage.tpl.php and clear cache. Now you'll see that the variables have disappeared.

This patch will ensure that previously discovered preprocess functions will get carried forward; and that new preprocess functions will still be discovered by keeping the 'original hook' name around and checking it during preprocess discovery.

(this is critical since it totally breaks the named-overrides that this feature was designed for).

Files: 
CommentFileSizeAuthor
#68 drupal-include_base_processor_files-241570.patch664 bytesedmund.kwok
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-include_base_processor_files-241570.patch. Unable to apply patch. See the log in the details link for more information. View
#61 241570-61-specific_template_preprocess_functions.patch2.48 KBbecw
PASSED: [[SimpleTest]]: [MySQL] 27,361 pass(es). View
#49 theme-registry-fix-d7-49.patch12.9 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 18,746 pass(es). View
#43 theme-registry-fix-d7-43.patch13.12 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 17,903 pass(es). View
#34 theme-registry-fix-d7-34.patch8.53 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 17,640 pass(es). View
#30 theme-registry-fix-d7-30.patch1.95 KBeffulgentsia
Passed on all environments. View
#14 theme-registry-fix-d7.patch2.1 KBCrell
Failed: 9679 passes, 1 fail, 0 exceptions View
#7 theme-registry-fix-2.patch1.62 KBmerlinofchaos
Failed: Failed to apply patch. View
fix-preprocess-patterns.patch1.68 KBmerlinofchaos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-preprocess-patterns.patch. View

Comments

ksenzee’s picture

I couldn't replicate this bug on a clean install of 6.x-dev. I tried garland and a contrib theme (sky) and I was able to get my custom views-view-unformatted--frontpage.tpl.php to show up fine in both themes. I visited /frontpage, added views-view-unformatted.tpl.php and stuck some text in there, and went back to /frontpage. My text showed up fine. I then renamed it to views-view-unformatted--frontpage.tpl.php, changed my custom text, cleared cache, and went back to /frontpage... and my new text was there. Let me know if I misunderstood the process or if you need any more info.

merlinofchaos’s picture

It isn't your text that won't appear, it's items that appeared in template_preprocess_views_view_unformatted. Which, actually, frontpage may be a bad choice because it doesn't do anything in the preprocess function. Change the view to a table, though. I picked frontpage because it comes with Views and should've actually tried that one myself.

merlinofchaos’s picture

For more detail, see http://drupal.org/node/241405 -- which is the issue that made me realize this was happening.

ksenzee’s picture

Thanks for the link -- I get it now. Yes, creating a table view does replicate the problem. It is a bit disconcerting to have all your content disappear. So bug replicated, patch tested, problem apparently solved. I even ran it through coder, just to be impudent. :)

ksenzee’s picture

Status: Needs review » Reviewed & tested by the community

As long as I'm being impudent, I'll mark it RTBC.

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev

Reread, reviewed again and committed to 6.x. Still to be committed to 7.x

merlinofchaos’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.62 KB
Failed: Failed to apply patch. View

Ok, it turns out (to my chagrin) that there was a problem with my previous patch. It turned out that adding the preprocess functions into the new hook now prevents preprocess functions from the theme from taking hold (so I ended up inverting the problem).

The reason for this is that if a theme sets preprocess functions specifically, it assumes that the theme did not want auto-discovery of the preprocess functions; so by copying them too early, we lose the discovery process.

The fix is to check to see if the original hook has preprocess functions and transfer them during the merge that happens after this check.

Sorry about this. I'll see if I can roll a merged patch for 7.x as well.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Testing Drupal 6.2 with both patches from this issue applied, and Views beta2. I am able to create both a template file and a preprocess function for a specific field in a view, by name. (That is, both of the issues here are addressed.)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Looking for another test before committing (to me, the patch looks just as good as the previous one looked :)

toby_panzer’s picture

these patches work for me using 6.2 and views beta2. Thanks everyone - especially Merlinofchaos, views is a wonderful addition to drupal. I can successfully create preprocess functions and views-view-fields--MYVIEW.tpl.php files to override theme settings.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

That's 2 confirmations.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

OK, committed the updated patch. Needs a reroll for Drupal 7 with both patches.

merlinofchaos’s picture

I just realized that I don't have a PHP 5.2 installation to roll a patch for HEAD on, so I am unable to do this. Or at least I cannot test a HEAD patch.

Crell’s picture

Status: Needs work » Needs review
FileSize
2.1 KB
Failed: 9679 passes, 1 fail, 0 exceptions View

Attached patch combines both of the above, rolled against today's HEAD.

visionmark’s picture

I still don't understand how to "install" this patch... Are those directions above? They don't say where to place this code....

merlinofchaos’s picture

jdblank’s picture

I applied the patch but it said 2 hunks failed and one succeeded. Any ideas?

cwgordon7’s picture

Are you applying this against HEAD?

aclight’s picture

As of today, the patch in #14 applies perfectly to HEAD, so the author of #17 must have been applying the patch to something else.

Crell’s picture

aclight: If it applies perfectly and works, please set RTBC so we can get this issue closed out. :-)

aclight’s picture

@Crell: I didn't say I had tested it :) I didn't have time for that, just to see that it applied properly.

Dries’s picture

#277626: Checking CCK nodes with drop down lists lead to SAX 0x0 parsing error might be suffering from this bug as well. For certain CCK types, Mollom receives a 0x0 ASCII/Unicode character which causes the server-side XML parser to barf with a error like An invalid XML character (Unicode: 0x0) was found. Where that 0x0 would slip into the output string isn't clear yet but KarenS thinks there could be relation with his bug. We can only reproduce it with CCK node types.

Crell’s picture

Dries: I am confused how the two could be related. This thread is just about finding .tpl files. Have you done any sort of before/after checks with this patch to see if it affects the other? If not, then I don't see how they'd be related.

ajaysolutions’s picture

Version: 7.x-dev » 6.3

Excuse my general ignorance, but I came to this thread from the Views support page because I'm having a problem with my Views page (called testimonials) not using the node-view-testimonials.tpl.php file it should be. It sometimes works, but when I change a configuration in Drupal, it breaks.

Using the Theme Developer (devel module) I can see that it's no longer even looking for my tpl.php file and has reverted to trying to find views-view-unformatted.tpl.php which isn't right.

I've tried applying the d7 patch from post #14 above, but that didn't work. The first two hunks succeeded and the second two failed. It looks to me like that code already exists (for the failed hunks) in the Drupal 6.3 theme.inc file but I'm not entirely sure what I'm supposed to be looking for.

What's the status of this problem? Or is the problem I'm experiencing nothing to do with this? Looking at the link merlinofchaos posted in #3 above, I think I'm on the right track...

Cheers,

Al

merlinofchaos’s picture

Al: I think that the problem is that the preprocess_node that views is using in node.views.inc is not always loaded when the theme registry is rebuilt, which is why you run into that. AS a workaround, move that function from node.views.inc into views.module (which I think is going to be my eventual solution, but I haven't fully committed to that yet).

ajaysolutions’s picture

Thanks merlinofchaos - I'll try that if/when I experience the issue again, but what I've done at the moment is the following:

- Taken your patch and applied it to my Drupal 6.3 (the first two hunks succeeded, the second two failed)
- Visited the modules page which should rebuilt the theme registry cache

This has fixed the problem this time around. I'm sure next time I make a config change to the server it'll lose it again, at which point I'll test your suggestion.

Much appreciated! I'll keep you posted.

Al,

dvessel’s picture

Version: 6.3 » 7.x-dev

This is still not in 7. Will test.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun.core’s picture

This needs tests.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
Passed on all environments. View

Reroll of #14.

effulgentsia’s picture

Yes, we need tests for this, but it would be very helpful for several other D7 issues being worked on for this to land quickly. This is a straight port of what's already been part of D6 for over a year.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I'm not too clear on what this does, but the bot its been on thousands of sites for over a year.

effulgentsia’s picture

The use-case for this comes in when theme() is called with an array of hooks (or now, with an implicit array through the use of the __ delimiter). For example, in menu_tree_output() we set $build['#theme_wrappers'][] = 'menu_tree__' . $data['link']['menu_name']; If someone were to actually implement a THEMENAME_menu_tree__MENUNAME() function, the template_preprocess_menu_tree() function would not run for it. This patch fixes that. This points to the need for better test coverage of all the awesome and crazy things the theme registry is capable of. I'll work on that when I get a chance, but I'm not sure when that will be, so if others want to work on that, please please do so. In the meantime, I'm not aware of anything currently that this is holding up (maybe the D7 port of views?), but as per #32, it's been in D6 for quite a while.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.53 KB
PASSED: [[SimpleTest]]: [MySQL] 17,640 pass(es). View

I have an alternate proposal for how to fix this in D7 instead of the forward port of the way it was fixed in D6.

In D7, we now use the 'pattern' concept and double-underscore convention for suggestions set in preprocess functions (what D6 did with $variables['template_files']): #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance, which I'm really pleased with for the reasons covered in that issue. But it introduces an interesting question: should there be any difference at all between specifying suggestions when calling theme() and specifying them from a preprocess function? With #30, there is a subtle difference: specifying suggestions when calling theme() invokes the preprocess/process functions of the suggestion, whereas specifying suggestions from a preprocess function does not, since at the time theme() needs to decide which preprocess/process functions to run, all it knows is the hook it was given. I'm not convinced that this is a useful distinction we want to support, and this patch removes such a distinction, resulting in no difference between suggestions specified when calling theme() and suggestions specified from a preprocess function, which I think will make things easier for non-expert developers. It leaves open the ability for a hook_theme_registry_alter() to unset the 'original hook' key, if it really wants to hack around this and customize the preprocess chain on a suggestion by suggestion basis (though this would also require such a module to use the pass suggestions to theme() approach over the specify suggestions in a preprocess function approach). But I really see this as an edge case. Anyone want to dispute this? Has anyone here implemented custom preprocess functions intended only to run on a single suggestion (e.g., hook_preprocess_views_view__VIEWNAME()) and feel that this is sufficiently superior to implementing hook_preprocess_views_view() that contains a switch statement on the view name so as to justify maintaining different behavior depending on where in the pipeline the suggestions are set?

moshe weitzman’s picture

Sounds like an edge case to me too. Consistency++

effulgentsia’s picture

#34: theme-registry-fix-d7-34.patch queued for re-testing.

effulgentsia’s picture

No objections to the added consistency but loss of edge-case support of #34 vs. #30. Given that, @moshe: are you comfortable RTBC'ing, or would you like to see a positive review from someone?

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

bot is happy.

Dries’s picture

To me this looks good, but admittedly, I'm not super familiar with this code. What is unclear, for example, is why we keep the 'original hook'. It does not seem to be used...

merlinofchaos’s picture

The original hook is used -- or at least it was once used -- to retrieve information that may actually not be needed any more with this retooling.

effulgentsia’s picture

Even with the re-tooling, it still has essentially the same purpose (see for example, the hunk that the patch adds to theme()). I would personally prefer the name 'base hook', but I refrained from making that change without merlinofchaos being around to comment on it. What do you think: should I re-roll with that name?

merlinofchaos’s picture

I have no preference between 'original hook' and 'base hook' but at this point it might be considered an API change and should probably not be changed without a solid argument (or an okay by Dries and/or webchick)

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.12 KB
PASSED: [[SimpleTest]]: [MySQL] 17,903 pass(es). View

I have no preference between 'original hook' and 'base hook' but at this point it might be considered an API change and should probably not be changed without a solid argument (or an okay by Dries and/or webchick)

This patch changes 'original hook' to 'base hook'. I don't think it's an API change, as at no point in the D7 development cycle, including currently in HEAD, has 'original hook' existed. This doesn't break anything in D7. It's only a change of name relative to what's in D6, but that's normal for a D6 -> D7 upgrade. And the argument for why this is a better name is the change in implementation relative to what's in D6. In this implementation, 'base' has a well known meaning (a suggestion is a theme hook that is a variant of some parent/ancestor/base hook), whereas it's not clear what meaning 'original' has ('original' sounds like something having to do with the internals of the order in which the registry processes things, which while having some truth to it, isn't the key concept that needs to be conveyed).

Downgrading to "needs review" because this patch also adds a test to demonstrate the bug fixed by this patch and so that it stays fixed. The test fails on HEAD, but passes with the patch. The test involves 4 new files, so webchick/Dries: make sure to "cvs add" those. Those are:

  • modules/simpletest/tests/theme_test.info
  • modules/simpletest/tests/theme_test.module
  • themes/tests/test_theme/template.php
  • themes/tests/test_theme/test_theme.info

So, the theme.inc hunks have already been RTBC'd, but I'd appreciate it if someone could review the test addition and RTBC if that looks good. Thanks.

merlinofchaos’s picture

I don't think it's an API change, as at no point in the D7 development cycle, including currently in HEAD, has 'original hook' existed.

You are slightly mistaken.

This is in Drupal 6 ( See http://drupalcode.org/viewvc/drupal/drupal/includes/theme.inc?revision=1... )

if (!empty($info['original hook']) && function_exists($prefix .'_preprocess_'. $info['original hook'])) {
  $info['preprocess functions'][] = $prefix .'_preprocess_'. $info['original hook'];
}

Now, this appears to have been removed in Drupal 7. I don't know by whom or where or when. I'm actually a little bit concerned by that, but it was in Drupal 6 somewhere around Drupal 6.4 I think.

effulgentsia’s picture

I think we're in agreement. It is in D6. My point is that it's not in D7. It was added to D6 after D7 development started, and never added to D7. This issue is now about adding it to D7, but I think that gives us the freedom to name the key what we want (for D7). I'm not suggesting that we make any changes to D6.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

So, the theme.inc hunks have already been RTBC'd, but I'd appreciate it if someone could review the test addition and RTBC if that looks good. Thanks.

No takers, huh? Since the code in #43 was already RTBC, and it's "just" tests that have been added, I'll go out on a limb and mark it RTBC myself. I hope the change from 'original hook' to 'base hook' helps answer #39.

catch’s picture

This is blocking other patches, it's a regression from D6, tests are a nice bonus, confirming RTBC.

rfay’s picture

subscribing.

aspilicious’s picture

FileSize
12.9 KB
PASSED: [[SimpleTest]]: [MySQL] 18,746 pass(es). View

Trying to chase head

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Though I like Dries am not super familiar with this code, I went ahead and committed this patch to HEAD. It's a regression from 6.x, it adds oodles of comments explaining what's going on in this murky part of the code, and it has test coverage to show that it's working properly. Yum.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

becw’s picture

Priority: Critical » Major
Status: Closed (fixed) » Active

I've been looking into #939462: Specific preprocess functions for theme hook suggestions are not invoked (see this comment)--exactly the "edge case" mentioned here in #34. I think that custom preprocess functions for specific suggestions are actually common in custom theming, as I've seen this method used in themes and even suggested in presentations (for example, slide #41 of Katherine Lynch's Theming Views: get it to "look right" presentation at DrupalCamp Chicago).

Given that this functionality was available in Drupal 6 (as mentioned in comment #44), I think this may be considered a regression.

MXT’s picture

Subscribing. I ran into the same problem on a specific template of Views (e.g. views-view--taxonomy-term-leaf--page.tpl): its preprocessing function mytemplate_preprocess_views_view__taxonomy_term_leaf__page(&$variables) has no effect on that template. Seems that the function is completely ignored.

sun’s picture

merlinofchaos’s picture

Note that Views doesn't do anything special here. Any actual regression (i.e, if there really is a bug and it is not just a configuration issue) then it's highly likely to be a Drupal core bug. All Views does is call theme() appropriately, and it registers a pattern so that Drupal's theme discovery can find the patterns. The button to rebuild the theme registry just calls Drupal's function to clear the theme registry so that it is automatialy rebuilt.

Unless the use of the 'pattern' key has changed and Views did not change with it, this may be a Drupal core bug. If that's the case, we will need to demonstrate this very carefully with specific instructions.

effulgentsia’s picture

There is a regression from D6, but an intentional one, as per #34/#35. When I get a chance, I'll read up on #52, and reply with any further thoughts.

merlinofchaos’s picture

Okay, I admit that I had never fully read #34 until this moment. At least, I have no memory of reading that patterns were prevented from having their own preprocess functions.

I am disappointed that this functionality was determined to be an edge case based on the input of 2, maybe 3 people. Since I wrote the functionality, I'm particularly disappointed that the removal of it was buried in an issue about something else, and it was not made completely crystal clear that functionality was being removed.

BOTH core maintainers admitted that they didn't really understand what was going on here. Shouldn't there have been larger agreement that the removed functionality isn't actually in use anywhere?

merlinofchaos’s picture

In any case, please, please please, put that functionality back.

Also, apologies about #55. At the time I read that, I actually thought I was reading the Views issue queue. I was distracted by real life at the time and didn't notice important details like the project being Drupal core.

Jacine’s picture

Please, please fix this. Please.

Crell’s picture

Priority: Major » Critical

It does sound like this is a regression from Drupal 6, and IMO a regression qualifies as critical.

becw’s picture

Status: Active » Needs review
FileSize
2.48 KB
PASSED: [[SimpleTest]]: [MySQL] 27,361 pass(es). View

Here's a possible patch. For theme hooks that declare a base hook, this code will make sure that preprocess and process functions from both hooks will run. However, templates that are chosen via patterns (like node--article.tpl.php) don't get preprocess functions via the same patterns (like template_preprocess_node__article())--I don't think that they did in Drupal 6, though.

This patch interleaves the preprocess and process functions; they run base preprocess, base process, specific preprocess, specific process (instead of preprocess, preprocess, process, process).

I'm also not sure where the 'override preprocess functions' => TRUE/FALSE key from hook_theme() comes in to play. It seems relevant to this issue, but is completely ignored at the moment.

JeremyFrench’s picture

I have not tested this, but the patch looks good in theory. Test bot likes it, but I am not sure if it tests preprocess functions extensively.

cosmicdreams’s picture

I'm willing to help test, but I haven't read through this entire thread to figure out what would be a good test. If you can describe how I can best test this patch I'll see if I can do that on Wednesday night!

becw’s picture

My patch is as much a question as a solution: what is the intended behavior here? As far as I know, the patch makes D7 behavior match D6, but I don't spend much time with the theme system, and unlike D6, theme function/template naming patterns are actually used in D7 core.

  • I think that 'override preprocess functions' should be checked to determine whether to run preprocess and process functions from the base hook. This would be a sensible addition to the patch above.
  • I don't know if the theme system should look for template preprocess functions that correspond with a particular theme hook pattern.
JeremyFrench’s picture

@bec, avoiding a regression is a good thing so your patch is more than a question. Reading the thread it looks like not having it is a regression.

I don't know if the theme system should look for template preprocess functions that correspond with a particular theme hook pattern.

I think it is a great idea. But I think it is too late for D7, it could potentially be backported as it would be a back compatible api change. It could also conceivably be done by a contrib. module.

I think that 'override pre-process functions' should be checked to determine whether to run pre-process and process functions from the base hook. This would be a sensible addition to the patch above.

That is a tricky one. As it stands there should be a bug that override pre-process functions is ignored. I suppose if this patch would cause that bug to be manifested then it should be included.

cor3huis’s picture

fix-preprocess-patterns.patch queued for re-testing.

effulgentsia’s picture

Status: Needs review » Closed (fixed)

Setting this back to closed as in #51. The original issue this thread was about was making sure hook_preprocess_views_view() ran when 'views-view--page-1.tpl.php' was used, and that has been fixed in both D6 and D7.

The discussion since then about the regression from D6 to D7 in that hook_preprocess_views_view__page_1() gets called in D6, but not in D7, is one worth having, but let's move it to #939462: Specific preprocess functions for theme hook suggestions are not invoked. I left a long comment there (#6) with my thoughts on the matter. I marked that issue major, rather than critical, because RC1 is not a deadline for it. If we decide we want D7 to work like D6 in this regard, that can be achieved without breaking BC for existing D7 code.

edmund.kwok’s picture

Priority: Critical » Major
Status: Closed (fixed) » Needs review
FileSize
664 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-include_base_processor_files-241570.patch. Unable to apply patch. See the log in the details link for more information. View

I'm reopening this because the file(s) specified to be included in the base hook is not added and the processor function will not run. Attached patch includes file(s) of the base hook.

Status: Needs review » Needs work

The last submitted patch, drupal-include_base_processor_files-241570.patch, failed testing.

catch’s picture

Status: Needs work » Closed (fixed)

@edmund.kwok. Please open a new issue, this was closed a long time ago.

Additionally all bug fixes should be posted againt 8.x, with the tag "Needs backport to D7" added.

effulgentsia’s picture

#1430300: Preprocess functions in an include file fail to get called when the theme implements a suggestion override now contains a test and fix for #68. For anyone interested, please review it, and RTBC if appropriate.