[drupal-devel] [task] Improve drupal theme system
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: stefan nagtegaal Status: patch 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. stefan nagtegaal
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.
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>';
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: Chris Johnson Status: patch Eh, what's wrong with using: $output .= "<div class='messages $type'>\n"; It works just fine from both the PHP and HTML perspectives. Chris Johnson 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.
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.
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 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. 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>'; ------------------------------------------------------------------------ April 14, 2005 - 16: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 - 17: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 - 12: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.
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: Chris Johnson Status: patch 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. Chris Johnson 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.
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.
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: factoryjoe Status: patch It seems to me that double quotes should always be used for quoting attributes in XHTML because most of my Javascript function calls use single quotes and using single quotes around them might cause problems. factoryjoe Previous comments: ------------------------------------------------------------------------ April 14, 2005 - 13: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 - 14: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 - 15: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 - 16: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 - 11: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 - 09: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 - 17: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. ------------------------------------------------------------------------ April 18, 2005 - 17:44 : jhriggs 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.
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: stefan nagtegaal Status: patch Attachment: http://drupal.org/files/issues/theme-inc.diff (741 bytes) - Removed deprecated function theme_error(); I searched the core, and theme_error() is not used anymore; - Added some helptext to theme_more_help_link(); please comment and apply.. stefan nagtegaal 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. ------------------------------------------------------------------------ April 16, 2005 - 13: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 - 11: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 - 19: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. ------------------------------------------------------------------------ April 18, 2005 - 19:44 : jhriggs 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. ------------------------------------------------------------------------ April 19, 2005 - 02:54 : factoryjoe It seems to me that double quotes should always be used for quoting attributes in XHTML because most of my Javascript function calls use single quotes and using single quotes around them might cause problems.
participants (6)
-
al -
Chris Johnson
-
Dries
-
factoryjoe
-
jhriggs
-
stefan nagtegaal