[drupal-devel] [task] Improve drupal theme system

[Dries]

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:   Dries
 Status:       patch

I dislike mange of the quote-related changes as well.  It makes the code
harder to read/study.  Won't commit as is.




Dries



Previous comments:
------------------------------------------------------------------------

April 14, 2005 - 15: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 - 16: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>';





------------------------------------------------------------------------

April 14, 2005 - 17:09 : al

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.




------------------------------------------------------------------------

April 14, 2005 - 18:55 : Chris Johnson

Eh, what's wrong with using:


$output .= "<div class='messages $type'>\n";



It works just fine from both the PHP and HTML perspectives.