r67278 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r67277‎ \| r67278 \| r67279 >
Date:	09:53, 3 June 2010
Author:	catrope
Status:	deferred (Comments)
Tags:
Comment:	Fixed for r58099 per CR: * Only clicktrack local, domain-relative URLs * Validate redirect URL in ApiClickTracking with the same condition used in ClickTracking.js (local, domain-relative) * Remove call to nonexistent function OutputPage::enable() * Add functionality for disabling API output and use this after setting up the redirect. This fixes the issue where the body of the redirect contained an API response in xmlfm form at; the body is now empty.
Modified paths:	/trunk/extensions/UsabilityInitiative/ClickTracking/ApiClickTracking.php (modified) (history) /trunk/extensions/UsabilityInitiative/ClickTracking/ClickTracking.js (modified) (history) /trunk/extensions/UsabilityInitiative/ClickTracking/ClickTracking.php (modified) (history) /trunk/phase3/includes/api/ApiFormatBase.php (modified) (history) /trunk/phase3/includes/api/ApiMain.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiFormatBase.php
—	—	@@ -36,7 +36,7 @@
37	37	abstract class ApiFormatBase extends ApiBase {
38	38
39	39	private $mIsHtml, $mFormat, $mUnescapeAmps, $mHelp, $mCleared;
40		~~- private $mBufferResult = false, $mBuffer;~~
	40	+ private $mBufferResult = false, $mBuffer, $mDisabled = false;
41	41
42	42	/**
43	43	* Constructor
—	—	@@ -114,12 +114,27 @@
115	115	}
116	116
117	117	/**
	118	+ * Disable the formatter completely. This causes calls to initPrinter(),
	119	+ * printText() and closePrinter() to be ignored.
	120	+ */
	121	+ public function disable() {
	122	+ $this->mDisabled = true;
	123	+ }
	124	+
	125	+ public function isDisabled() {
	126	+ return $this->mDisabled;
	127	+ }
	128	+
	129	+ /**
118	130	* Initialize the printer function and prepare the output headers, etc.
119	131	* This method must be the first outputing method during execution.
120	132	* A help screen's header is printed for the HTML-based output
121	133	* @param $isError bool Whether an error message is printed
122	134	*/
123	135	function initPrinter( $isError ) {
	136	+ if ( $this->mDisabled ) {
	137	+ return;
	138	+ }
124	139	$isHtml = $this->getIsHtml();
125	140	$mime = $isHtml ? 'text/html' : $this->getMimeType();
126	141	$script = wfScript( 'api' );
—	—	@@ -172,6 +187,9 @@
173	188	* Finish printing. Closes HTML tags.
174	189	*/
175	190	public function closePrinter() {
	191	+ if ( $this->mDisabled ) {
	192	+ return;
	193	+ }
176	194	if ( $this->getIsHtml() ) {
177	195	?>
178	196
—	—	@@ -191,6 +209,9 @@
192	210	* @param $text string
193	211	*/
194	212	public function printText( $text ) {
	213	+ if ( $this->mDisabled ) {
	214	+ return;
	215	+ }
195	216	if ( $this->mBufferResult ) {
196	217	$this->mBuffer = $text;
197	218	} elseif ( $this->getIsHtml() ) {
Index: trunk/phase3/includes/api/ApiMain.php
—	—	@@ -199,6 +199,13 @@
200	200	public function getModule() {
201	201	return $this->mModule;
202	202	}
	203	+
	204	+ /**
	205	+ * Get the result formatter object. Only works after setupExecuteAction()
	206	+ */
	207	+ public function getPrinter() {
	208	+ return $this->mPrinter;
	209	+ }
203	210
204	211	/**
205	212	* Only kept for backwards compatibility
—	—	@@ -330,7 +337,7 @@
331	338
332	339	header( "Cache-Control: $ccHeader" );
333	340
334		~~- if ( $this->mPrinter->getIsHtml() ) {~~
	341	+ if ( $this->mPrinter->getIsHtml() && !$this->mPrinter->isDisabled() ) {
335	342	echo wfReportTime();
336	343	}
337	344
Index: trunk/extensions/UsabilityInitiative/ClickTracking/ClickTracking.js
—	—	@@ -10,8 +10,8 @@
11	11	$j(document).ready( function() {
12	12	$( '#p-logo a, #p-navigation a, #p-interaction a, #p-tb a' ).each( function() {
13	13	var href = $(this).attr( 'href' );
14		~~- // Only modify local and same-schema URLs~~
15		~~- if ( href[0] == '/' \|\| href.match( /^https?:\/\// ) ) {~~
	14	+ // Only modify local URLs
	15	+ if ( href.length > 0 && href[0] == '/' && ( href.length == 1 \|\| href[1] != '/' ) ) {
16	16	var id = 'leftnav-' + skin + '-' + ( $(this).attr( 'id' ) \|\| $(this).parent().attr( 'id' ) );
17	17	href = wgScriptPath + '/api.php?action=clicktracking' +
18	18	'&eventid=' + id + '&token=' + wgTrackingToken + '&redirectto=' + escape( href );
Index: trunk/extensions/UsabilityInitiative/ClickTracking/ApiClickTracking.php
—	—	@@ -49,10 +49,20 @@
50	50	// For links that go off the page, redirect the user
51	51	// FIXME: The API should have a proper infrastructure for this
52	52	if ( !is_null( $params['redirectto'] ) ) {
53		~~- global $wgOut;~~
54		~~- $wgOut->enable();~~
55		~~- $wgOut->redirect( $params['redirectto'] );~~
56		~~- $wgOut->output();~~
	53	+ // Validate the redirectto parameter
	54	+ // Must be a local URL, may not be protocol-relative
	55	+ $href = $params['redirectto'];
	56	+ if ( strlen( $href ) > 0 && $href{0} == '/' && ( strlen( $href ) == 1 \|\| $href{1} != '/' ) ) {
	57	+ global $wgOut;
	58	+ $wgOut->redirect( $params['redirectto'] );
	59	+ $wgOut->output();
	60	+
	61	+ // Prevent any further output
	62	+ $wgOut->disable();
	63	+ $this->getMain()->getPrinter()->disable();
	64	+ } else {
	65	+ $this->dieUsage( 'The URL to redirect to must be domain-relative, i.e. start with a /', 'badurl' );
	66	+ }
57	67	}
58	68	}
59	69
Index: trunk/extensions/UsabilityInitiative/ClickTracking/ClickTracking.php
—	—	@@ -19,7 +19,7 @@
20	20	/* Configuration */
21	21
22	22	// Increment this value when you change ClickTracking.js
23		~~-$wgClickTrackingStyleVersion = 4;~~
	23	+$wgClickTrackingStyleVersion = 5;
24	24
25	25	// click throttle, should be seen as "1 out of every $wgClickTrackThrottle users will have it enabled"
26	26	// setting this to 1 means all users will have it enabled

Follow-up revisions

Revision	Commit summary	Author	Date
r68228	Followup to r67278: use [0] not {0} for string indexing per CR. Decided again...	catrope	18:06, 18 June 2010

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r58099	ClickTracking: Also track navigation links in left sidebar. This means that C...	catrope	20:45, 24 October 2009

Comments

#Comment by Bryan (talk | contribs) 12:38, 3 June 2010

I think it is more intuitive to have an isEnabeled() function than an isDisabled() one.

#Comment by Tim Starling (talk | contribs) 14:58, 8 June 2010

Brace syntax for string offsets is said to be deprecated, use square brackets instead. The whole line is rather verbose:

+			if ( strlen( $href ) > 0 && $href{0} == '/' && ( strlen( $href ) == 1 || $href{1} != '/' ) ) {

May I suggest strspn( $href, '/' ) == 1?

The use of $wgOut in an API module appears to be unique. I'm OK with it as a temporary thing, but ideally I think ApiMain should have a helper function that calls header() directly. It would be more consistent.

Otherwise good.

#Comment by Catrope (talk | contribs) 15:01, 8 June 2010

I never knew which one of $str{1} and $str[1] was the better one, thanks for clearing that up.

Didn't know about strspn() either, thanks for the hint. That, in combination with $href[0] == '/' should be good enough, right?

About the $wgOut use: yes, this is not ideal and should be done better eventually, hence // FIXME: The API should have a proper infrastructure for this a few lines up.

#Comment by Tim Starling (talk | contribs) 15:09, 8 June 2010

No, just strspn($href,'/') == 1. It will be zero if $href[0] != '/', so you don't need that condition. Same for a zero-length string.

The manual is a bit imprecise. Basically, it's equivalent to:

function strspn( $subject, $mask ) {
    for ( $i = 0; $i < strlen( $subject ) && strpos($mask, $subject[$i]) !== false; $i++);
    return $i;
}

It doesn't skip an initial non-matching part.

#Comment by Catrope (talk | contribs) 15:13, 8 June 2010

Yeah, I see it now, the manual does allude to this, but only all the way at the bottom:

Returns the length of the initial segment of str1 which consists entirely of characters in str2.

#Comment by Tim Starling (talk | contribs) 15:22, 8 June 2010

strspn() and strcspn() are worth keeping in mind when you're doing PHP string processing. If you like regexes, you could think of them as faster versions of /^[$mask]*/ and /^[^$mask]*/ respectively.

Status & tagging log

19:39, 21 November 2010 Reedy (talk | contribs) changed the status of r67278 [removed: new added: deferred]