[drupal-devel] [feature] modifcation to allow theme engines to
theme all theme hooks
gordon
drupal-devel at drupal.org
Mon Mar 7 04:34:04 UTC 2005
Issue status update for http://drupal.org/node/16409
Project: Drupal
-Version: cvs
+Version: 4.5.0
Component: theme system
Category: feature requests
Priority: normal
Assigned to: gordon
Reported by: gordon
Updated by: gordon
Status: patch
No that was option 2, and I really would prefer not for modify the core
theme engine to get access to additional theming functions.
The more you have to modify the theme engine to get access to functions
the greater possiblity that something is going to go wrong during an
upgrade.
I prefer to keep clients as standard as possible. This means that
upgrades are easier and cost less. which then means it is easier to
maintain all my clients as they are all using the same version of
drupal. This translates to a lower TCO (total cost of ownership) for my
clients.
gordon
Previous comments:
------------------------------------------------------------------------
January 30, 2005 - 08:31 : gordon
Attachment: http://drupal.org/files/issues/theme.inc_1.patch (889 bytes)
Attached is a patch that I would like to get into 4.6. I didn't realise
it but I am cutting this close.
What this patch does is give the ability to theme engines to be able to
handle all theme hooks with specifically having to code for them by
creating a function {theme engine}_{theme hook} (eg. phptemplate_pager)
What I have done to achieve this is I have created a new theme engine
hook called theme_handler. This will check to see if the current theme
has been set up to include this theme hook, and returns the function
that it should called to handle this theme hook. Most the time this
would most likely be a generic theme function.
This function is then called with exactly the same arguements as the
theme() was called with. (eg the theme hook as the first arguement) So
in the case of the pager the function is called like so
<?php
phptemplate_generic("pager", "", 10)
?>
The method that I have used, theme engines are not required to use this
functionality, as some theme engines such as I think xtemplate will not
be able to handle this, but it works extremely well in phptemplate.
The reason that I have made this modification is that i can use a theme
engine such as phptemplate to create the theme, and if I want to theme
any additional features such as pager, menu_tree, I can without having
to modify the theme engine from standard. So this gives me the best of
both worlds.
So with the phptemplate engine all I need to do to make use of the
additional functionality is add template file to the themes directly
like:-
hook = hook.tpl.php, pager = pager.tpl.php, etc.
It also means in the case of the phptemplate engine the theme creaters
php skills can be limited, and still be able to expand the theme beyond
that of the engines capibilities.
------------------------------------------------------------------------
January 30, 2005 - 08:32 : gordon
Attachment: http://drupal.org/files/issues/phptemplate_0.patch (1.12 KB)
and here is the patch for the phptemplate engine to make use of this
functionality.
------------------------------------------------------------------------
January 31, 2005 - 03:10 : FactoryJoe at civicspacelabs.org
It seems to me that this functionality belongs in PHPTemplate and not
another theme engine.
Are there any possible security issues raised with using such an
open-ended patch?
------------------------------------------------------------------------
January 31, 2005 - 22:34 : Anonymous
This was part of my original design for the new theme systes, but was
removed because it was considered unneccesary complexity that only
affected phptemplate.
I think this is the right time to add it however, as it allows lots of
really creative things.
Also, most of the available template engines (smarty , phptal ,
phptemplate) are based on my original phptemplate code base, and as
such will benefit from this patch.
------------------------------------------------------------------------
January 31, 2005 - 22:48 : adrian
Bah.
I wasn't logged in.
Anyway, to answer joe's question .. this needs to be modified in core
so that phptemplate can leverage it.
It really is only a small change in theme.inc.
------------------------------------------------------------------------
February 1, 2005 - 06:10 : gordon
Looking at the patch yes I do think there maybe a little security
concern. But I am not exactly sure of the best method to employ.
1. I could just return from the theme_handler a true/false and then
always call {theme engine}_generic and it can work it all out, which in
the case of phptemplate is all that it needs to call.
or
2. have the theme_handler return the hook, and then execute it as
{theme engine}_{hook} which does give you the added flexibility, but it
is not as much of a security issue.
Also as said before, we change to core is required so that hooks that
are not know by the theme engine have some way of being called.
------------------------------------------------------------------------
February 2, 2005 - 14:53 : gordon
Attachment: http://drupal.org/files/issues/theme.inc_2.patch (908 bytes)
I have made a change to the patch so that the process that is called is
called {theme engine}_{returned hook} which I feel is more secure, but
gives you more flexibility if the theme engine has the default handles
split up for ease of use.
------------------------------------------------------------------------
February 2, 2005 - 14:54 : gordon
Attachment: http://drupal.org/files/issues/phptemplate_1.patch (1.11 KB)
and the patch for phptemplate
------------------------------------------------------------------------
February 3, 2005 - 12:37 : adrian
Ok.
I have (possibly) a spanner to throw in the works. The variables passed
to the template appear in which format?
I recall now why I didn't include this functionality. Essentially, the
template handling code accepts an associative array, and passes each of
the elements in the array to the template as a variable .. hence :
<?php
$vars = array(
'varA' =>'text 1',
'varB' => 'text 2',
'varC' => 'text 3);
?>
Is available internally in the template as :
<?php
$varA = 'text 1';
$varB = 'text 2';
$varC = 'text 3';
?>
Because of how php (and most c based languages work), the called
function has to name it's parameters, and with a generic function, the
best you can do is numbered variables. Of course, $1 isn't a valid
variable name, so the best that could be done is $var1, $var2 , $var3
or something similar.
To combat this would probably require rewriting the theme() function to
accept an associative array, but the amount of bloat and opportunity for
error that causes is phenomenal. The primary reason I am not using a
generic template passing function, is that I can't pass well named
variables to the template.
------------------------------------------------------------------------
February 3, 2005 - 13:02 : gordon
Yes I know that you cannot create any meaningful variable names which is
why in for the phptemplate_generic() I only export a variable called
$args which includes all the arguements that have been passed to the
generic hook.
This means that it can handle all unknown themeing functions without
having to modify the theme engine which makes my life alot easier in
the future when I have to upgrade client sites.
Any ways anything beyond the current theme hooks that are provided by
phptemplate do require some expert knowledge to make sure that they
done stuff up. It also means that you can have the simplicity of the
phptemplate engine to create your theme and be able to theme everything
that you desire.
An example of this is that I want to use phplayersmenu to format my
navigation block. At this stage I have created a module called navtree
which does this for me, but with this patch I am able to create a
menu_tree.tpl.php and I can add in phplayersmenu in the theme and not
have to have a little module that does it for me.
------------------------------------------------------------------------
February 21, 2005 - 12:59 : Chris Johnson
+1
This seems like a good addition to me. It provides theme developers a
lot more flexibility without having to become module authors as well.
------------------------------------------------------------------------
February 21, 2005 - 16:55 : bryankennedy
+1 - i agree this would be super cool for 4.6
------------------------------------------------------------------------
February 27, 2005 - 10:24 : Steven
One possible way to get around the naming issue might be to store the
parameters of all themable functions and name them the generic
arguments that way.
Of course it has to be maintained whenever themable functions change,
but you could do this with a script which regexpes for theme_ functions
and extracts the argument names:
<?php
global $list;
function iterate($path) {
$d = opendir($path);
if ($d) {
while ($f = readdir($d)) {
$n = $path .'/'. $f;
if (preg_match('/\.(inc|module)$/', $f)) {
$data = file_get_contents($n);
parse($data);
}
else if (is_dir($n) && ($f{0} != '.')) {
iterate($n);
}
}
closedir($d);
}
}
function parse($source) {
global $list;
static $done = array();
preg_match_all('/function (theme_.*?)\(.*?\)/', $source,
$functions, PREG_SET_ORDER);
foreach ($functions as $v) {
$name = $v[1];
if (isset($done[$name])) {
continue;
}
$done[$name] = 1;
preg_match_all('/\$([A-Za-z0-9_]+)/', $v[0], $args,
PREG_SET_ORDER);
$item = array();
foreach ($args as $arg) {
$item[] = "'". $arg[1] ."'";
}
$list[] = " '$name' => array(". implode(", ", $item) .")";
}
}
$list = array();
iterate(".");
print "array(\n". implode(",\n", $list) .");\n";
?>
------------------------------------------------------------------------
February 27, 2005 - 17:40 : gordon
this is great, The biggest problem is keeping the list up to date, and
for people who have written there own custom theme functions they will
have to alter the theme engine themselves which is what I am trying to
avoid with this patch. You could turn this into a standard function
that can be called from the generic function. You would added caching
and lower the overhead. But still in the end what is it really doing
for you, instead of having $args[] you would have a the real variable
name, which will make it pretter but no additional functionality.
I do like this but other than creating a nicer set of varables for when
calling generic theme functions what does this really give you. This
seems like alot of overhead when you can just reference $args[0].
------------------------------------------------------------------------
February 28, 2005 - 05:00 : Steven
True, but the whole point of PHPTemplate is to make theming 'nicer':
without the separate templates and easy variables, it's the same as a
.theme file.
Anyway it was just an idea I had. It's not critical, but I do think not
having named variables will make templates a lot harder to make and
read.
------------------------------------------------------------------------
March 6, 2005 - 13:00 : gordon
I am really wanting to get this patch applied to standard for 4.6.
At this stage the main concerns have been with the implementation into
the theme engine, and not the changes to core. I propose that we apply
the changes in core, and then either apply my patch to phptemplate or
someone can create some other method of implementing into the theme
engines.
Just to hilight why I feel this is such an important change, I will
give you my current situation. Basically I have a client that I am
making changes to the aggregator module, only in the method of how it
displays the item, and not really how change how the aggregator module
works.
In the current senerio, I can do 1 of 2 things.
modify the aggragator module to output the aggregator_page_item the way
my client wants it.
modify the phptemplate engine to allow the theming of the
agregator_page_item, and the provide a aggregator_page_item.tpl.php
file for the theme.
Both of these requires either changes to the core of drupal, or changes
to the theme engine. Both of these I do not find acceptable when triing
to keep clients as standard as possible so that upgrading from one
version of drupal to another will not become cost prohibitive, and end
up having to support customers on multiple versions of drupal.
what this small patch does is allow me to send a just the
aggregator_page_item.tpl.php to the client and requires no changes to
either core, or the template engine. It gives me the flexibility of
developing a theme using a template engine which should have a lower
cost, with the power of a native theme (ie. not using a theme engine).
I know that the arguments that are being sent to it are not the
prettiest in my implementation to the phptemplate, but it does give you
all the power, and flexibility that you could ever want.
------------------------------------------------------------------------
March 6, 2005 - 23:46 : adrian
It appears there is a third scenario you aren't aware of.
You don't have to hack the template engine to add the code.
You can create a template.php file in the theme directory and specify
the following :
<?php
function phptemplate_pager_item($item) {
return _phptemplate_callback('pager_item', array('item' => $item));
}
?>
You are now distributing 2 files instead of one, but it gives you
cleanly named variables. Allowing files to be picked up automatically
is nice and all, but it can be set up for templates without modifying
the core files already.
It's in the documentation here : http://drupal.org/node/11811
This allows for fully distributable additional templates, and while I
dislike the fact that this 3 line stub of php is needed for extra
templates, it does make the templates themselves easier to write.
Oh. and i removed the additional template caching from phptemplate in
4.6, as file_exists was nowhere near as expensive as I thought, hence
this patch doesn't apply anymore. Also, as phptemplate is destined for
core in 4.7, this will need to be solved properly (with proper variable
names) before this will be added.
More information about the drupal-devel
mailing list