Template code is incorrectly placed, missing check.
| Project: | Javascript Aggregator |
| Version: | 5.x-0.6 |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | derjochenmeyer |
| Status: | closed |
Jump to:
Hi!
This module seems to work fine, and I'm glad to see that contingency has been made for troublesome modules (who could have thought that TinyMCE would be a problem...? :p )
It strikes me though, that putting pure php code into the page template is almost entirely inappropriate. The $scripts variable is created by the phptemplate engine, and as such can be easily overridden in the template.php file.
I therefore propose that rather than have people hacking at their page.tpl.php files (naughty!) you should advise them to insert the following code into the template.php file. This should be placed in _phptemplate_variables, specifically in the 'page' section.
if(module_exists('javascript_aggregator') && $vars['scripts']) {
$vars['scripts'] = javascript_aggregator_cache($vars['scripts']);
}This code implements the template.php style of using the theme variables, namely $vars['phptemplate_variable'], and also includes a check to make sure there was actually a script in the first place, saving a potentially wasteful function call - very niche case admittedly!
Hope this helps. I have also tested this quite well (including with jquery_update) and it seems fine.
Nik
Web Development in Nottingham, UK by Kineta Systems
#1
Bah, forgot to add: another advantage of this is that if you have more than one page template (maybe four or five?) you'd have to update *each* of those files with the original code, whereas this is a single file update only.
#2
Great! I didnt have the time to investigate this... But its however a little more complex to setup then... now its easy "copy this code there"...
Modifying the _phptemplate_variables respectively adding it if it doesnt exist could be hard for new or unskilled users..
#3
This is what we would have to add (replace) in the documentation and on the description page, right?
Place the following code in your themes template.php file in the page section of the function _phptemplate_variables()
if(module_exists('javascript_aggregator') && $vars['scripts']) {$vars['scripts'] = javascript_aggregator_cache($vars['scripts']);
}
If that function does not exist in your template.php file add it by pasting the follwoing code
/**
* Override or insert PHPTemplate variables into the templates.
*/
function _phptemplate_variables($hook, $vars) {
if ($hook == 'page') {
if(module_exists('javascript_aggregator') && $vars['scripts']) {
$vars['scripts'] = javascript_aggregator_cache($vars['scripts']);
}
return $vars;
}
return array();
}
If your theme does not come with a template.php file create an empty text file and name it template.php them past the code above with an <?php opening tag! and save it to the directory of your theme. Check the core module garland for an example of a template.php
#4
#5
You're quite correct in that it is slightly more difficult to add in the template.php file, but it's the "Drupal way", and is also general best practise.
Any themer will tell you that keeping the template files as free as possible from php markup is the Way Forward.
The content in #4 looks pretty good to me! :)
#6
#7
#8
Automatically closed -- issue fixed for two weeks with no activity.