r102587 MediaWiki - Code Review archive

• I already fixed the echo's (leftover from debugging?) in the followup
• Could you add some slightly more informative comments than "Step A/Step B/etc?"

Thanks for the echo fix. I was trying to debug something, but I couldn't see the output and forgot about it. Do you know if there is any way to see the names of tests as they are executed? Or if there is any specific way to debug tests?

The comments map directly to the details in the RFC. You are right though, I will add some non-normative remarks inline.

There might be some verbosity options within phpunit that we could play with :)

Ok, I looked through the documentation I could find, but I didn't see any. It looks like we need to implement an entirely new PHPUnit_Util_Printer. :-/

Use --tap to show the test names :-)

Example output:

$ ./phpunit.php --tap
TAP version 13
ok 1 - ArticleTablesTest::testbug14404
ok 2 - ArticleTest::testImplementsGetMagic
ok 3 - ArticleTest::testImplementsSetMagic
ok 4 - ArticleTest::testImplementsCallMagic
ok 5 - ArticleTest::testGetOrSetOnNewProperty
ok 6 - ArticleTest::testStaticFunctions
...

if( preg_match('%^\.\.?/%', $urlPath, $matches) ) {

# Step A $urlPath = substr( $urlPath, strlen( $matches[0] ) );

}

Will match './' which is matched in Step B as well. You could just use substr() == to compare. Would save you a preg_match call and a strlen call (since '../' is known to be 3 chars).

} elseif ( preg_match( '%^/\.(/|$)%', $urlPath, $matches ) ) {

# Step B $start = strlen( $matches[0] ); $urlPath = '/' . substr( $urlPath, $start );

Same there, isn't strlen( $matches[0] ) always 3 here? Plus setting $start and using it only once after is not that helpful.

I am wondering if there is an existent PEAR implementation for this algorithm. Will avoid us reinventing the wheel and creating new bugs. We could ship the upstream lib in ./includes/libs and still run test on it.

Step A matches ./ and ../ each only if they are at the beginning of the string. Step B matches /./ or /. followed by the end of the string.

Both steps are matching two different possible strings.

Even if they were each matching only one, I am pretty sure a preg_match anchored to the beginning of the string while using only ? as a wildcard and | as a combinator followed by a boolean test is much faster than creating new strings and running a string comparison. Anchored, greedy regular expressions are generally quite fast.

If you think this implementation is particularly slow or in a critical path, perhaps we should run a performance test.

The usage of $start keeps the subsequent line under 80 characters. They can certainly be inlined if that is the preferred style.

If you find an implementation in PEAR, please let me know. I did a fair bit of searching for a PHP implementation of this RFC. The best attempt I could find was a ragged/"clever" patch for something Drupal related.

I have found an implementation for removing dot segment in PEAR. The module is Net::URL2 : http://svn.php.net/viewvc/pear/packages/Net_URL2/trunk/Net/URL2.php?view=markup Look for 'function removeDotSegments'.

Nice find. It appears correct.

Feel free to swap it in if you prefer.

Actually, please patch out the counter if you do. It opens a security vulnerability.

not really a FIXME, more of a todo.

not really a FIXME, more of a todo.

Consuming a string by repeatedly taking trailing substrings is not an appropriate way to parse in PHP, since it necessarily leads to O(N^2) runtime. substr() is O(N) in the size of the remaining substring, and you do O(N) substrings, so the overall runtime is O(N^2). Most string functions in PHP have an offset parameter which allow you to avoid this style of string processing.

Note that, at least last time I tested it, anchoring a regex with "^" doesn't work with an offset parameter, so the /A modifier needs to be used instead.

So you will have a style like:

$p = 0;
while ( $p < strlen( $urlPath ) ) {
   if ( preg_match( '%...%A', $urlPath, $matches, 0, $p ) ) {
      ...
      $p += strlen( $matches[0] ); 
   }
   ...
}

+	while ( $urlPath ) {

Do not convert strings to boolean like this, because strings such as "0000" will convert to false. See Manual:Coding conventions/PHP#Pitfalls for more tips like this.

I think using substr() for your leading substring comparisons, like in Hashar's comment, would improve performance slightly. You could save a few substrings to local variables:

$lead1 = substr( $urlPath, $p, 1 );
$lead2 = substr( $urlPath, $p, 2 );
$lead3 = substr( $urlPath, $p, 3 );

then most of your regexes would just become comparisons between these local variables and string literals. The regex in step E could be replaced with an strpos() search for "/". It would probably also improve the readability of the code, since your regexes are fairly dense.

Thanks for the detailed feedback. :-)

I have posted a followup, but I messed up the linking. http://www.mediawiki.org/wiki/Special:Code/MediaWiki/109720