r60421 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r60420‎ | r60421 | r60422 >
Date:23:15, 26 December 2009
Author:overlordq
Status:reverted (Comments)
Tags:
Comment:
Followup to r56514 r56174 empty strings are not NULL
Modified paths:
  • /trunk/phase3/includes/Xml.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Xml.php
——@@ -43,7 +43,7 @@
4444 */
4545 public static function expandAttributes( $attribs ) {
4646 $out = '';
47 - if( is_null( $attribs ) ) {
 47+ if( empty( $attribs ) ) {
4848 return null;
4949 } elseif( is_array( $attribs ) ) {
5050 foreach( $attribs as $name => $val )

Follow-up revisions

RevisionCommit summaryAuthorDate
r61904Revert r56514 and r60421 per CR.tstarling04:54, 3 February 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r56174Fixes for r56068 and r55984:...ialex10:59, 11 September 2009
r56514Followup to r56174: tooltipAndAccesskey also returns a stringcatrope14:22, 17 September 2009

Comments

#Comment by Tim Starling (talk | contribs)   08:12, 12 January 2010

empty() is just negated conversion to boolean with errors suppressed. The following two lines of code are equivalent: 

if ( empty( $atribs ) )

if ( !$atribs ) {

except that the second one will give you a warning about an undefined (misspelled) variable name and the first will not. This is why I have so regularly ranted, on wikitech-l (http://marc.info/?l=wikitech-l&m=122160666518029&w=2) and elsewhere, against the use of empty() except where the intent really is to suppress errors.

However, in this case, !$attribs would be wrong too, since that would be true for strings like '0.0' where something else might be intended. Instead, I think this and r56514 should be reverted and Xml::expandAttributes() should continue to throw an exception if it's passed a string.

Status & tagging log