r82218 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r82217‎ | r82218 | r82219 >
Date:06:27, 16 February 2011
Author:catrope
Status:reverted (Comments)
Tags:
Comment:
Followup r82156: skip relative URLs with absolute paths too
Modified paths:
  • /trunk/phase3/includes/libs/CSSMin.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/libs/CSSMin.php
——@@ -120,8 +120,8 @@
121121 self::URL_REGEX . '(?P<post>[^;]*)[\;]?/';
122122 $offset = 0;
123123 while ( preg_match( $pattern, $source, $match, PREG_OFFSET_CAPTURE, $offset ) ) {
124 - // Skip absolute URIs
125 - if ( preg_match( '/^https?:\/\//', $match['file'][0] ) ) {
 124+ // Skip absolute URIs and relative URIs with absolute paths
 125+ if ( preg_match( '/^(\/|https?:\/\/)/', $match['file'][0] ) ) {
126126 // Move the offset to the end of the match, leaving it alone
127127 $offset = $match[0][1] + strlen( $match[0][0] );
128128 continue;

Sign-offs

UserFlagDate
RobLa-WMFinspected06:30, 16 February 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r822201.17wmf1: MFT r80495, r80765, r81177, r82000, r82155, r82156, r82191, r82200,...catrope07:23, 16 February 2011
r82456Revert r82218 , doesn't fix absolute path URLs but breaks them. Will fix prop...catrope14:46, 19 February 2011
r82457(bug 27328) Redo r82218 properly, expanding URLs with absolute pathscatrope14:58, 19 February 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r82156Resolves remaining issues in r82000 with remapping relative CSS URLs that con...tparscal00:49, 15 February 2011

Comments

#Comment by Nikerabbit (talk | contribs)   07:30, 16 February 2011

~^(/|https?://)~ would be more readable.

#Comment by Darklama (talk | contribs)   12:54, 18 February 2011

What does "relative URIs with absolute paths" mean? There seems to be a problem with the logic here. Thanks to this change url("/w/index.php") is treated as an absolute URI.

#Comment by Catrope (talk | contribs)   15:05, 18 February 2011

It means a URL such as "/w/index.php" , so that's what I intended. Is there anything wrong with that?

#Comment by Darklama (talk | contribs)   18:49, 18 February 2011

Yes. "/w/index.php" needs to be remapped to the right website for @import to work correctly as reported in bug 27328.

Status & tagging log