Issue status update for http://drupal.org/node/20498 Project: Drupal Version: cvs Component: theme system Category: tasks Priority: normal Assigned to: stefan nagtegaal Reported by: stefan nagtegaal Updated by: al Status: patch I completely agree with jhriggs on this. So much so, in fact, that I've come out of hiding to say as much. And if your argument for making the code much less readable by putting in single quotes is "it's faster", you'll find there are plenty of places in Drupal where your time would be better spent optimising big chunks of slowness rather than imperceptible improvements like how your string substitution works. -1 on these sorts of string changes. al Previous comments: ------------------------------------------------------------------------ April 14, 2005 - 14:47 : stefan nagtegaal Attachment: http://drupal.org/files/issues/theme-inc.patch (7.1 KB) Attached patch: - uses single quotes instead of double quotes where possible; - removes a deprecated theme function called theme_error() (we now use theme_set_message($message, $type) for that); More improveents to come, when this is in core. ------------------------------------------------------------------------ April 14, 2005 - 15:50 : jhriggs A big -1 on a lot of the string changes. I am not a fan of changing to single quotes just to change to single quotes. We should pick what is more readable which is what Dries has told me in the past. Changes like this are not a step forward: - return "<img src=\"$path\" $attr alt=\"$alt\" title=\"$title\" />"; + return '<img src="'. $path .'" '. $attr .' alt="'. $alt .'" title="'. $title .'" />'; That is just really hard to read. The escaped double quotes aren't the best, but it is much easier to read than a lot of concatenation with mixed single and double quotes. I am also opposed to all of the newlines that were removed. It may make the output HTML a tad smaller, but it makes the resulting HTML much harder to read and debug. Example of a bad change: - $output .= "<div class=\"messages $type\">\n"; + $output .= '<div class="messages '. $type .'">'; if (count($messages) > 1) { - $output .= " <ul>\n"; + $output .= '<ul>'; foreach($messages as $message) { - $output .= ' <li>'. $message ."</li>\n"; + $output .= '<li>'. $message .'</li>'; } - $output .= " </ul>\n"; + $output .= '</ul>'; } else { $output .= $messages[0]; } - $output .= "</div>\n"; + $output .= '</div>';