r92005 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r92004‎ | r92005 | r92006 >
Date:20:55, 12 July 2011
Author:catrope
Status:ok (Comments)
Tags:
Comment:
Fix r14202 (!!): this validates the protocol against the regex for the second time (preg_split has already made sure it matches wfUrlProtocols() so no need), and uses strpos() to validate against a regex (that's just wrong). I'm removing it not because it's useless but because it breaks for URL protocols that don't contain a ':' ('//' in my case) for no reason at all. Found out about this when writing parser tests for it (will commit these in a minute), so yay tests!
Modified paths:
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/Parser.php
——@@ -1540,16 +1540,10 @@
15411541
15421542 # No link text, e.g. [http://domain.tld/some.link]
15431543 if ( $text == '' ) {
1544 - # Autonumber if allowed. See bug #5918
1545 - if ( strpos( wfUrlProtocols(), substr( $protocol, 0, strpos( $protocol, ':' ) ) ) !== false ) {
1546 - $langObj = $this->getFunctionLang();
1547 - $text = '[' . $langObj->formatNum( ++$this->mAutonumber ) . ']';
1548 - $linktype = 'autonumber';
1549 - } else {
1550 - # Otherwise just use the URL
1551 - $text = htmlspecialchars( $url );
1552 - $linktype = 'free';
1553 - }
 1544+ # Autonumber
 1545+ $langObj = $this->getFunctionLang();
 1546+ $text = '[' . $langObj->formatNum( ++$this->mAutonumber ) . ']';
 1547+ $linktype = 'autonumber';
15541548 } else {
15551549 # Have link text, e.g. [http://domain.tld/some.link text]s
15561550 # Check for trail

Sign-offs

UserFlagDate
Tim Starlinginspected07:28, 5 September 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r92006Parser tests for protocol-relative URLs, for r91663 and r92005catrope20:56, 12 July 2011
r921781.17wmf1: MFT r92005, another prot rel URL fixcatrope18:44, 14 July 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r14202Fix #5918: links autonumbering now work for all defined protocols.hashar19:29, 13 May 2006

Comments

#Comment by Hashar (talk | contribs)   21:00, 12 July 2011

Wonderful!!! Great to see bugs getting smashed thanks to testing.

As stated on IRC, please make sure there is no regression for bug 5918.

[ftp://foobar] is autonumbered. [ftp://foobar ftp://foobar] is still a free link.

#Comment by Catrope (talk | contribs)   01:44, 13 July 2011

CodeReview mangled your comment, so it's not quite clear what you meant, but the parser test for bug 5918 still passes. I guess another way of saying it is that bug 5918 applied to protocol-relative URLs before this change.

Status & tagging log