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

[jhriggs]

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

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>';





jhriggs



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

April 14, 2005 - 07: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.