r111350 MediaWiki - Code Review archive

if ( mb_strlen( $str ) > $this->ItemMaxLength ) {
					$str = mb_substr( $str, 0, $this->ItemMaxLength ) . " ...";
			}

Be careful here - PHP may not necessarily be compiled with mb string support as it is a non-default extension. Before relying on mb functions, perhaps ensure they exist with function_exists().

Also, nitpicky style note:

+		# Get a maximal length for item texts
 	80	+		switch ( true ) {
 	81	+		case ( isset( $args['item-max-length'] ) ):
 	82	+			$this->ItemMaxLength = $args['item-max-length'];
 	83	+			break;
 	84	+		case ( isset( $wgRSSItemMaxLength ) ):
 	85	+			$this->ItemMaxLength = $wgRSSItemMaxLength;
 	86	+			break;
 	87	+		}
 	88	+		

Indentation is messed up here. Also, this seems like an unnecessarily more difficult to read version of using if() {} elseif(){}.

"Be careful here - PHP may not necessarily be compiled with mb string support as it is a non-default extension. Before relying on mb functions, perhaps ensure they exist with function_exists(). "

No, it is not. As already mentioned in the comment, GlobalFunctions.php automatically wraps the mb_ function if it does not exists. The indentation is perfect. The switch is a very fine layout here.

Reset to NEW

Ah indeed - I missed the comment line and didn't know we had those functions overloaded in GlobalFunctions.php.

As for the indentation, I think convention dictates that the case() statements should be indented an additional level from switch().

And as for even using the switch() statement this way, I think it's just a matter of style - personally I find this much more confusing when reading through code quickly than using if() {} elseif().

Source / and my Reference:

https://www.mediawiki.org/w/index.php?title=Manual:Coding_conventions&oldid=426301

There are some exceptions, such as switch statements, where the indentation of the cases are optional, so both of the below are fine.

        switch ( $mimetype ) {
        case 'text/xml':
        case 'application/xhtml+xml':
        case 'application/xml':
                return true;
        default:
                return false;
        }

        switch ( $mimetype ) {
                case 'text/xml':
                case 'application/xhtml+xml':
                case 'application/xml':
                        return true;
                default:
                        return false;
        }

The usage in those examples is vastly differently from how you are doing that.

The usage in those examples is vastly differently from how you are doing that."

fixed in r111816 .

Note however, the Language class provides a truncate method which can localize the '...' message ( $parser->getFunctionLang()->truncate( $str, $length ); ), which may be a better choice. OTOH that truncate method is more meant for byte limits, so would handle multibyte characters slightly differently, so I suppose mb_substr is ok if we want the length to be the number of "code points" so to speak.

One of the lines of the case statement:

84 + case ( isset( $wgRSSItemMaxLength ) ):

is a little confusing at first glance, since isset( $wgRSSItemMaxLength ) should always return true. (Not a major issue, and is probably ok, just a bit confusing)

Indeed, you are right, because $wgRSSItemMaxLength is always set in RSS.php it should be existent, but I wanted to be sure for cases where admins set or, worse, unset $wgRSSItemMaxLength in their LocalSettings.

BTW: this line was further changed in follow-up r111351 to

• case ( isset( $wgRSSItemMaxLength ) && is_numeric( $wgRSSItemMaxLength ) )

GlobalFunctions:

if ( !function_exists( 'mb_strlen' ) ) { 51 /** 52 * @codeCoverageIgnore 53 * @return int 54 */ 55 function mb_strlen( $str, $enc = ) { 56 return Fallback::mb_strlen( $str, $enc ); 57 } 58 }

Fallback:

/** 129 * Fallback implementation of mb_strlen, hardcoded to UTF-8. 130 * @param string $str 131 * @param string $enc optional encoding; ignored 132 * @return int 133 */ 134 public static function mb_strlen( $str, $enc = ) { 135 $counts = count_chars( $str ); 136 $total = 0; 137 138

[CORR] using preformatted

GlobalFunctions.php:

50 	if ( !function_exists( 'mb_strlen' ) ) {
51 	/**
52 	* @codeCoverageIgnore
53 	* @return int
54 	*/
55 	function mb_strlen( $str, $enc = '' ) {
56 	return Fallback::mb_strlen( $str, $enc );
57 	}
58 	}

Fallback.php

/**
129 	* Fallback implementation of mb_strlen, hardcoded to UTF-8.
130 	* @param string $str
131 	* @param string $enc optional encoding; ignored
132 	* @return int
133 	*/
134 	public static function mb_strlen( $str, $enc = '' ) {
135 	$counts = count_chars( $str );
136 	$total = 0;
137 	
138