[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

I believe you are right, Chris.  I looked into it too.  It seems XML
attributes can be double or single quoted.  I guess my only reservation
with using single-quoted attributes would be consistency.  Most
everywhere we use double-quoted attributes.  It might reduce
readability from a consistency standpoint.




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.




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

April 14, 2005 - 08: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 - 09: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 - 10: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.




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

April 16, 2005 - 05:55 : Dries

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




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

April 18, 2005 - 03:25 : al

Chris Johnson wrote:
> Eh, what's wrong with using:
> $output .= "\n";


Correct me if I'm wrong, but that's not how XML is formed, and
therefore it wouldn't validate as XHTML.




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

April 18, 2005 - 11:35 : Chris Johnson

Al, are you talking about the newline appended to the end of the string?
 I just quoted the code that was previously quoted from the patch, and
changed the quoting of the string and the (X)HTML attributes.  As far
as I know (having quickly glanced at the W3 documents), single and
double quotes are valid for attributes, which is what I was trying to
demonstrate.  You might be right about the use of newline, but that
wasn't really part of my intended point.


Note also:  In PHP, '$foo' is a string while "$foo" is an interpolated
variable.  However, "some '$foo' text" results in $foo being
interpolated as a variable, that is, PHP is smart.  This characteristic
allows general use of single quotes inside double quote strings for
attributes.


As a result, the \" mess can be cleaned up without resorting to the
concatenation mess proposed in the original patch.


But it's just a suggestion, or point of information.  Anything that
makes code easier to read makes it easier to maintain and results in
fewer bugs.