Posted by philbar on March 25, 2010 at 6:51pm
Hey Guys,
I'd appreciate it if you could help test out a module I just put up on drupal.org.
http://drupal.org/project/csstidy
Thanks!
Also, if we decide to do a meet-up next month, I can give a presentation on website performance optimization.
Comments
Just to let you know. I see
Just to let you know. I see that you packaged up css tidy into the module. Even though the code is gpl this is considered bad practice in drupal land. (Even though my module does it. It was already added to the package before I started co-maintaining it.) It is best if you get people to download css tidy and give them instructions on how to install it in the sites/all/plugins folder. Look at wysiwyg to see how they do that.
Thanks for the heads up. I'm
Thanks for the heads up. I'm very familiar with the policy.
I just can't justify adding an extra step and burdening the site builder. I think this policy was created to prevent non-GPL code in d.o CVS and keeping the repository small and manageable.
Depending on some issues in Drupal core, I may be modifying CSSTidy and trying to get it into D8 as an
.incto replace the regexp CSS Optimization incommon.incfile. CSSTidy has been abandoned by it's maintainer so new versions won't be polluting the CVS.Really, if they want to enforce this policy, they should offer some solutions to the installation hassle. I've worked on the following, but they have all stalled:
Our next meetup will be in
Our next meetup will be in May and it would be great to hear a presentation on performance.
Bob Kepford
TheWeeklyDrop.com
BobKepford.com
Hi Phillip, Glad to see
Hi Phillip,
Glad to see another Fresno Drupal member developing modules! I tested this out (sorry it took so long) and it gave me a 29% improvement on my CSS, which is great. That said, I'd like to offer some insight into your module code.
Those are are my tidbits. I'll leave it up to you to put in the issue cue if you agree. :) Overall, great job and I'll be looking forward to future releases!
@lowtrak - I'm not familiar
@lowtrak - I'm not familiar with
in_array(). I tried just replacingarray_search()within_array()and it broke my site. What is your suggestion regarding "zero-index logic bugs"?Here is the code again for reference:
<?php/**
* Implementation of hook_theme_registry_alter().
*
* Make csstidy's page preprocess function run after everything else.
* If the css_gzip module is installed, move it's preprocess function after ours.
*/
function csstidy_theme_registry_alter(&$theme_registry) {
if (isset($theme_registry['page'])) {
// Move our preprocess function after everything else.
if ($key = array_search('csstidy_preprocess_page', $theme_registry['page']['preprocess functions'])) {
unset($theme_registry['page']['preprocess functions'][$key]);
}
$theme_registry['page']['preprocess functions'][] = 'csstidy_preprocess_page';
// Move css_gzip's preprocess function after ours.
if ($key = array_search('css_gzip_preprocess_page', $theme_registry['page']['preprocess functions'])) {
unset($theme_registry['page']['preprocess functions'][$key]);
$theme_registry['page']['preprocess functions'][] = 'css_gzip_preprocess_page';
}
}
}
?>
To use in_array() instead of
To use in_array() instead of array_search() as the condition for the if statement, you can do something like this...
<?php/**
* Implementation of hook_theme_registry_alter().
<em>
</em> Make csstidy's page preprocess function run after everything else.
* If the css_gzip module is installed, move it's preprocess function after ours.
*/
function csstidy_theme_registry_alter(&$theme_registry) {
if (isset($theme_registry['page'])) {
// Move our preprocess function after everything else.
if (in_array('csstidy_preprocess_page', $theme_registry['page']['preprocess functions'])) {
$key = array_search('csstidy_preprocess_page', $theme_registry['page']['preprocess functions']);
unset($theme_registry['page']['preprocess functions'][$key]);
}
$theme_registry['page']['preprocess functions'][] = 'csstidy_preprocess_page';
// Move css_gzip's preprocess function after ours.
if (in_array('css_gzip_preprocess_page', $theme_registry['page']['preprocess functions'])) {
$key = array_search('css_gzip_preprocess_page', $theme_registry['page']['preprocess functions']);
unset($theme_registry['page']['preprocess functions'][$key]);
$theme_registry['page']['preprocess functions'][] = 'css_gzip_preprocess_page';
}
}
}
?>
The problem is.. if $key is ever returned as the value of 0, the condition of the if statement will be false.
To expand on what jerryjr
To expand on what jerryjr mentioned, if csstidy_preprocess_page is the first item in the 'preprocess functions' registry array, it will return 0 as the index and therefore set $key = 0. Anything that evaluates to zero as a condition in PHP is the same as FALSE.
In your code, if csstidy_preprocess_page is the first item, it will fail the if condition and set it again without removing it from the array (now you have the item set twice in the array). This may have unintended consequences. Since the purpose of that code is to move csstidy_preproces_page to bottom of the array, I assume making sure it runs last is important.
Consider using jerryjr's code, which handles the checks properly.