Now that we have modules in their own directories, it would be great to extract all the theme functions out of the .module file and put them into a .theme file that lies in the module directory.

A little quote from my mailing list post:

Why we should move theme_* functions into a [modulename].theme file:

* ease pain for designers. Currently, they have to look through all the module code, that is not interesting for them at all, to find a theme function they want to overwrite. By having them all together in a .theme file, it will save a lot of time and effort for them. And IMO we should take a bit more care of them, especially if it does not hurt at all.

* Seperation of code and design has always been a main target. So I think it's only logical to stick to this policy here and move the V part of a module into a different file than the C part (according to the MVC pattern)

I wrote an extractor script that uses some regular expressions to achieve this. Thanks to drupal's amazing code quality, not one error or expection did occur! ;-) (As far as I can see).
To test it yourself, just place the extract.php in your drupal root and run it. The extractor script is added as attachment.

I also added a little convenience function to include files that are located in a module directory (can also be used for other files that a module might want to include, i.e. a module-specific .inc file). Patch for this will be in the next follow-up (is needed to run drupal when extract.php was run).

Everything works completely well for me so far, so I think we should try to get this into 4.8. Designers will thank us...

regards,
frando

CommentFileSizeAuthor
#12 common_16.patch916 bytesFrando
#11 extract.php_0.txt4.32 KBFrando
#1 common.inc_28.patch886 bytesFrando
extract.php.txt4.32 KBFrando

Comments

Frando’s picture

Frando commented
StatusFileSize
new common.inc_28.patch886 bytes

This patch adds a little function that can be used to include files that belong to a module (=are located in the module directory).

I wrote it to include the .theme files, but it might also be useful for other purposes, such as adding .inc files that lie in a module directory (contrib uses this quite a lot).

rickvug’s picture

rickvug commented

Work on this looks to be going down on the Bryght code repository, which can be seen here. I remember hearing something about the end result being about 5 - 10% slower than before, which was too large of a hit to be included in core as is. Possibly you can work through some of the issues and get this effort rolling again. I know it would be a large improvement for designers. You will probably want to contact Adrian (I think) about this as he'll be in the know.

Frando’s picture

Frando commented

I did a quick performance test. I loaded the frontpage with several blocks and nodes 500 times with Jmeter, one time without and one time with my patch. The average load time was 64ms without and 66ms with my patch, so it doesn't have any significant on performance.

I'm not too familiar with what increases php performance and what decreases it, so it might be possible that in different testing scenarios one include(...) more or less has more influence, but I don't think so.

Some more reviews/comments would be appreciated.

morbus iff’s picture

morbus iff
they/them
commented

I'd be interested in seeing those performance results at 5000 and 50,000 times. This has to scale.

Wouldn't it make more sense to include the theme only when it's needed? That way, we get a slight speedup when we do things that don't 'em (like _help, nodeapi calls that only add to node but not modify it, etc.).

Actually, I'm wondering if we could hook that into theme() - if the passed theme isn't in memory, load all theme files to look for it. If we demand proper namespaces on the theme's names, we can then only load the actual namespace'd file.

Frando’s picture

Frando commented

Yes, I did also think about including theme files only if they're needed. This could probably improve performance, as there are often modules that don't have any output for a specific page request.

To achieve that, we had to add another parameter to theme(..), becaue currently we specify only the name of the theme function and not the module to which the theme file belongs. This is needed to know which theme file has to be included if function_exists(theme_whatever) returns FALSE).

The patch to theme(..) should be no problem, however, this would require quite a lot of code change, though (every theme function call needs to get updated).

Frando’s picture

Frando commented

Of course, if all theme functions are named properly (theme_[module]_[whatever]), existing code would not have to be changed.

merlinofchaos’s picture

merlinofchaos commented

would this be a good time to consider adding theme registration callback? Instead of theme_ functions, have a hook_themes, which associates theme 'functions' with a file.

Then theme, instead of looking for a function, looks for the appropriate tpl.php file -- first in the theme directory, then in the directory of the module that registered it. If no module registered it, it looks in the theme directory anyway (for more dynamic stuff) and that's all it has to do.

That wouldn't require changes to calls to theme() and is not a bigger change (i don't think) than what this patch is currenly doing. It speeds the theme() calling process from what we have now even, and means tpl.php files are only ever loaded on demand.

Thoughts?

Frando’s picture

Frando commented

Hmm,
I definitely like the idea of having .tpl.php-files that control the output of a page request.

The drupal core modules have between zero and 13 theme functions (comment.module has 13).
Do we want to have 13 .tpl.php files in modules/comment/theme? Would performance increase if we have 13 .tpl.php file in contrast to having one .theme file?

Frando’s picture

Frando commented

Hmm
the bigger problem I see is:

What do we do with the theme-functions that are not defined in modules, but in the various .inc-files?

I did a quick hack to the theme function to show wether the called function is defined in a .module or in a .inc file, and the result for visiting the front page with several nodes on it was the following:

Theme function called, defined in a .inc:    node
Theme function called, defined in a .inc:    links
Theme function called, defined in a .inc:    username
Theme function called, defined in a .inc:    pager
Theme function called, defined in a .inc:    pager_first
Theme function called, defined in a .inc:    pager_previous
Theme function called, defined in a .inc:    pager_list
Theme function called, defined in a .inc:    pager_next
Theme function called, defined in a .inc:    pager_link
Theme function called, defined in a .inc:    pager_last
Theme function called, defined in a .inc:    page
Theme function called, defined in a .inc:    blocks
Theme function called, defined in a .inc:    menu_tree
Theme function called, defined in a .inc:    menu_item
Theme function called, defined in a .inc:    menu_item_link
Theme function called, defined in a .module: user_list
Theme function called, defined in a .inc:    block
Theme function called, defined in a .inc:    breadcrumb
Theme function called, defined in a .inc:    closure
Theme function called, defined in a .inc:    help
Theme function called, defined in a .inc:    status_messages
Theme function called, defined in a .inc:    menu_local_tasks

And for a single node with a comment:

Theme function called, defined in a .inc:    node
Theme function called, defined in a .inc:    links
Theme function called, defined in a .inc:    username
Theme function called, defined in a .module: comment_thread_expanded
Theme function called, defined in a .inc:    pager
Theme function called, defined in a .module: comment_wrapper
Theme function called, defined in a .inc:    page
Theme function called, defined in a .inc:    blocks
Theme function called, defined in a .inc:    menu_tree
Theme function called, defined in a .inc:    menu_item
Theme function called, defined in a .inc:    menu_item_link
Theme function called, defined in a .module: user_list
Theme function called, defined in a .inc:    block
Theme function called, defined in a .inc:    breadcrumb
Theme function called, defined in a .inc:    closure
Theme function called, defined in a .inc:    help
Theme function called, defined in a .inc:    status_messages
Theme function called, defined in a .inc:    menu_local_tasks
Theme function called, defined in a .inc:    menu_local_task

Multiple calls for the same theme function are not shown to keep the list short.

But the result is quite clear: the vast majority of all theme functions called are not defined in .module but in .inc files, so we should do some thinking what to do with them resp. where to put them.

Crell’s picture

Crell commented

This is something I'd been thinking aloud about on the Devel mailing list earlier today. I definitely think there's a great deal of flexibility to be gained by moving each theme function to its own file. I am not certain what the performance difference would be, though.

As I mentioned on the list, we don't need namespacing. We survive just fine now without it. All we'd need to do is have the admin/modules page, which already updates the system table for all found modules, to also record the location of any theme files it finds. theme() then just checks for a function/file in the active theme directory and uses that if found, or goes for the file in the system table if not. Very themer-friendly.

If the performance hit is not severe, then it may make sense to shift the core .inc theme functions/files to the system module, as it's guaranteed included every time. That means less code to parse on every page load in core. If the performance difference is significant and cannot be overcome with caching or similar, then we may need some split logic in theme() to call a function if it exists or go the file route if it doesn't.

Also, note that node, page, blocks, block, and comment are generally overridden by a template engine anyway in practice. The built-in functions are rarely used, I believe. (All the more reason to not have to parse them!)

Frando’s picture

Frando commented
StatusFileSize
new extract.php_0.txt4.32 KB

It would still be awesome to get this in ...
such a little change, but it would ease many things a lot and could pave the way for further improvement of the theme system.

I've rerolled everything against latest head. Attached to this post is the extractor script. See comments there for explanations. Just place it in you drupal root and run it (writing permissions needed - it's probably best to run it via the command line).
If I knew how to create a patch that adds new files, I would save you from dealing with this script. Unfortunately, I don't. But the script works ;)

In my next post will be a little patch against common.inc that is also needed.

For me it is RTBC as I can't see any bugs anymore, but some more reviews would be nice ...

Frando’s picture

Frando commented
StatusFileSize
new common_16.patch916 bytes

n/t

Frando’s picture

Frando commented
Status: Needs review » Postponed

Setting this to postponed for now.
I'll probably revive it for 5.1 ...

forngren’s picture

forngren commented

Subscibing while the issue is sleeping :D

Frando’s picture

Frando commented
Version: x.y.z » 6.x-dev
Status: Postponed » Needs review

Moving to correct queue.

chx’s picture

chx commented

The code in Bryght repo wanted to do one file per theme function -- actually one .tpl.php per theme function, and this is a very nice idea. This is not bad either -- we shall see what comes out of it. But yes, contacting Adrian (his user id is so easy to remember and so like him: 1337) is a good idea.

add1sun’s picture

add1sun commented

subscribing

Crell’s picture

Crell commented
Status: Needs review » Closed (duplicate)

The theme registry patch for Drupal 6 handles this issue in a much nicer way. Yay!

senpai’s picture

senpai commented
Status: Closed (duplicate) » Closed (fixed)

FYI: The Theme Registry Patch issue can be found at http://drupal.org/node/130987. Marking this duplicate issue as fixed, since the original issue has been committed.