Theme preprocess functions do not get retained when using patterns

merlinofchaos - April 2, 2008 - 00:03
Project:Drupal
Version:7.x-dev
Component:theme system
Category:bug report
Priority:critical
Assigned:Unassigned
Status:needs review
Description

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).

AttachmentSizeStatusTest resultOperations
fix-preprocess-patterns.patch1.68 KBIdleFailed: Failed to apply patch.View details | Re-test

#1

ksenzee - April 3, 2008 - 13:18

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.

#2

merlinofchaos - April 3, 2008 - 15:08

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.

#3

merlinofchaos - April 3, 2008 - 15:09

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

#4

ksenzee - April 3, 2008 - 16:54

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. :)

#5

ksenzee - April 3, 2008 - 17:12
Status:needs review» reviewed & tested by the community

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

#6

Gábor Hojtsy - April 25, 2008 - 20:55
Version:6.x-dev» 7.x-dev

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

#7

merlinofchaos - May 14, 2008 - 18:37
Status:reviewed & tested by the community» needs review

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.

AttachmentSizeStatusTest resultOperations
theme-registry-fix-2.patch1.62 KBIdleFailed: Failed to apply patch.View details | Re-test

#8

Crell - May 14, 2008 - 18:46
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.)

#9

Gábor Hojtsy - May 14, 2008 - 18:50
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 :)

#10

toby_panzer - May 15, 2008 - 17:21

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.

#11

Crell - May 15, 2008 - 17:25
Status:needs review» reviewed & tested by the community

That's 2 confirmations.

#12

Gábor Hojtsy - May 15, 2008 - 18:08
Status:reviewed & tested by the community» needs work

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

#13

merlinofchaos - May 15, 2008 - 19:14

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.

#14

Crell - May 28, 2008 - 23:17
Status:needs work» needs review

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

AttachmentSizeStatusTest resultOperations
theme-registry-fix-d7.patch2.1 KBIdleFailed: 9679 passes, 1 fail, 0 exceptionsView details | Re-test

#15

visionmark - June 5, 2008 - 18:10

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

#16

merlinofchaos - June 5, 2008 - 18:16

#17

jdblank - June 7, 2008 - 22:24

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

#18

cwgordon7 - June 18, 2008 - 21:38

Are you applying this against HEAD?

#19

aclight - July 1, 2008 - 11:22

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.

#20

Crell - July 1, 2008 - 14:48

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

#21

aclight - July 1, 2008 - 16:36

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

#22

Dries - July 4, 2008 - 07:57

#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.

#23

Crell - July 6, 2008 - 00:27

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.

#24

ajaysolutions - August 7, 2008 - 16:46
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

#25

merlinofchaos - August 7, 2008 - 17:59

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).

#26

ajaysolutions - August 8, 2008 - 10:11

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,

#27

dvessel - November 23, 2008 - 17:59
Version:6.3» 7.x-dev

This is still not in 7. Will test.

#28

System Message - February 5, 2009 - 12:25
Status:needs review» needs work

The last submitted patch failed testing.

#29

sun.core - January 8, 2010 - 01:48

This needs tests.

#30

effulgentsia - January 14, 2010 - 18:18
Status:needs work» needs review

Reroll of #14.

AttachmentSizeStatusTest resultOperations
theme-registry-fix-d7-30.patch1.95 KBIdlePassed on all environments.View details | Re-test

#31

effulgentsia - January 14, 2010 - 18:23

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.

#32

moshe weitzman - January 14, 2010 - 18:54
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.

#33

effulgentsia - January 20, 2010 - 20:54

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.

#34

effulgentsia - January 22, 2010 - 19:02
Status:reviewed & tested by the community» needs review

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?

AttachmentSizeStatusTest resultOperations
theme-registry-fix-d7-34.patch8.53 KBIdlePassed on all environments.View details | Re-test

#35

moshe weitzman - January 22, 2010 - 19:17

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

 
 

Drupal is a registered trademark of Dries Buytaert.