The theme system is supposed to accomodate include files, but there seems to be a bug in line 234 in function _theme_process_registry
It requires one to specify both 'file' and 'function' if you want to use an include file, rather than the function name being generated as normal.
Attached patch is one simple approach - use a 'include file' element instead to specify include files.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | template_file_fix_3.patch | 12.74 KB | dvessel |
| #9 | template_file_fix_2.patch | 11.62 KB | pwolanin |
| #8 | template_file_fix_1.patch | 3.2 KB | pwolanin |
| inc_theme_1.patch | 881 bytes | pwolanin |
Comments
Comment #1
Crell commentedIt's an API change, but it is a fully backward compatible one and makes the API cleaner for optimizing code. It breaks nothing, so I'm OK with it but also wouldn't be heart-broken if it doesn't go in.
Comment #2
merlinofchaos commentedChange this from 'include file' back to 'file' and then change the uses of 'file' that mean 'template' to 'template'. That'll create a bunch of clarity, and I approve of that.
ACtually, I'm ambivalent about 'file' vs 'include file'.
Comment #3
profix898 commentedRegarding 'file' vs. 'include file'. We have an issue about .inc files and batches at http://drupal.org/node/169079. Its not really related, but maybe we can make the property consistent in menus, theme functions and batches!?
Comment #4
pwolanin commentedsince the menu system uses 'file', I'd like to go with merlin's 'template' and 'file'.
Comment #5
Crell commented'file' vs. 'template' sounds good and consistent to me. Would that also affect preprocessing functions? I suppose it would if it's read as "include this file before you do anything with this theme entity".
Comment #6
pwolanin commentedActually - that's a very good question. It seems that a even a theme function with a template might need to specify an include file that contains the preprocess function? So both 'file' and 'template' may be set of the same theme function?
Comment #7
merlinofchaos commentedYes -- 'file' could have meaning both ways, which is actually the bug that makes this need fixing and pushes it, to me, from an API change to a bugfix.
Comment #8
pwolanin commentedok - here's a starter patch to *look* at - changes theme.inc as described by Merlin. Allows both templates and functions to specify a file (an include file) as 'file' while a template file name is now 'template'.
Comment #9
pwolanin commentedfull patch- obviously you need to rebuild the theme registry (I just deleted everything in {cache})
Initial testing seems to work fine.
Comment #10
dvessel commentedI didn't test every page load for each theme function but scanned through the registry. All the "files" point to .inc files and "template" to templates.. Also looked at each HOOK_theme() that used templated functions and it's all good.
So, marking as RTBC.
Comment #11
Crell commentedLooks OK from a read through, but I don't have time to apply and test right now. Remember that the a.d.o page for hook_theme will have to be updated!
Comment #12
dvessel commentedOne thing that should be addressed in this patch are conditionally included files. The registry will only work for function/preprocessors that exist when going through its discovery process.
I had a problem before when I moved "template_preprocess_block_admin_display()" into block.admin.inc. (Was my first template conversion that shouldn't have been committed. :-)
It would be a minor addtion to include the "file" just to be on the safe side. I mentioned this to Merlinofchaos and he said it's okay.
Comment #13
dvessel commentedHere it is to include any files when building the registry. Tested by moving the preprocess function back into block.admin.inc. Works like a charm.
The poll.module entries were off so I changed them too.
Comment #14
pwolanin commentedTested this from install onwards - everything seems to work fine. I enabled all the modules touched by the patch and tested them as well.
Poll module still seems rather broken - but that's not caused or changed by this patch.
So, I think this is good.
Comment #15
merlinofchaos commentedWith two solid tests this is good to go.
Comment #16
dries commentedI agree that this is an important change/fix. Committed to CVS HEAD.
Comment #17
(not verified) commented