r56116 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r56115‎ | r56116 | r56117 >
Date:22:26, 9 September 2009
Author:dale
Status:resolved (Comments)
Tags:
Comment:
* (bug 20336) changed json_decode json_encode to static class in global functions
** we should update extensions as well
* added config for fileCheckModify date check to scriptLoader unique script id generation
Modified paths:
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/api/ApiFormatJson.php (modified) (history)
  • /trunk/phase3/includes/filerepo/ForeignAPIRepo.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadFromChunks.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadFromChunks.php
——@@ -167,7 +167,7 @@
168168 // c) (we need the token to validate chunks are coming from a non-xss request)
169169 $token = urlencode( $wgUser->editToken() );
170170 ob_clean();
171 - echo ApiFormatJson::getJsonEncode( array(
 171+ echo FormatJson::encode( array(
172172 'uploadUrl' => wfExpandUrl( wfScript( 'api' ) ) . "?action=upload&".
173173 "token={$token}&format=json&enablechunks=true&chunksessionkey=".
174174 $this->setupChunkSession( $summary, $comment, $watch ) ) );
——@@ -179,7 +179,7 @@
180180 // firefogg expects a specific result per:
181181 // http://www.firefogg.org/dev/chunk_post.html
182182 ob_clean();
183 - echo ApiFormatJson::getJsonEncode( array(
 183+ echo FormatJson::encode( array(
184184 'result' => 1,
185185 'filesize' => filesize( $this->getRealPath( $this->mTempAppendPath ) )
186186 )
——@@ -209,7 +209,7 @@
210210 // firefogg expects a specific result per:
211211 // http://www.firefogg.org/dev/chunk_post.html
212212 ob_clean();
213 - echo ApiFormatJson::getJsonEncode( array(
 213+ echo FormatJson::encode( array(
214214 'result' => 1,
215215 'done' => 1,
216216 'resultUrl' => $file->getDescriptionUrl()
Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -3344,3 +3344,27 @@
33453345 $langCode = implode ( '-' , $codeBCP );
33463346 return $langCode;
33473347 }
 3348+class FormatJson{
 3349+ public static function encode($value, $isHtml=false){
 3350+ // Some versions of PHP have a broken json_encode, see PHP bug
 3351+ // 46944. Test encoding an affected character (U+20000) to
 3352+ // avoid this.
 3353+ if (!function_exists('json_encode') || $isHtml || strtolower(json_encode("\xf0\xa0\x80\x80")) != '\ud840\udc00') {
 3354+ $json = new Services_JSON();
 3355+ return $json->encode($value, $isHtml) ;
 3356+ } else {
 3357+ return json_encode($value);
 3358+ }
 3359+ }
 3360+ public static function decode($value, $assoc=false){
 3361+ if (!function_exists('json_decode') ) {
 3362+ $json = new Services_JSON();
 3363+ $jsonDec = $json->decode($value);
 3364+ if($assoc)
 3365+ $jsonDec = (array) $jsonDec;
 3366+ return $jsonDec;
 3367+ } else {
 3368+ return json_decode($value, $assoc);
 3369+ }
 3370+ }
 3371+}
\ No newline at end of file
Index: trunk/phase3/includes/filerepo/ForeignAPIRepo.php
——@@ -130,7 +130,7 @@
131131 }
132132 $this->mQueryCache[$url] = $data;
133133 }
134 - return json_decode( $this->mQueryCache[$url], true );
 134+ return FormatJson::decode( $this->mQueryCache[$url], true );
135135 }
136136
137137 function getImageInfo( $title, $time = false ) {
Index: trunk/phase3/includes/OutputPage.php
——@@ -227,34 +227,49 @@
228228 * gets the scriptLoader javascript include
229229 * @param $forcClassAry Boolean: false by default
230230 */
231 - function getScriptLoaderJs( $forceClassAry = false ){
 231+ function getScriptLoaderJs( $classAry = array() ){
232232 global $wgRequest, $wgDebugJavaScript;
233 -
234 - if( !$forceClassAry ){
235 - $class_list = implode( ',', $this->mScriptLoaderClassList );
236 - } else {
237 - $class_list = implode( ',', $forceClassAry );
 233+ //if no class array provided use the mScriptLoaderClassList var
 234+ if( !count($classAry) ){
 235+ $classAry = $this->mScriptLoaderClassList;
238236 }
 237+ $class_list = implode(',', $classAry);
239238
240239 $debug_param = ( $wgDebugJavaScript ||
241240 $wgRequest->getVal( 'debug' ) == 'true' ||
242241 $wgRequest->getVal( 'debug' ) == '1' )
243242 ? '&debug=true' : '';
244243
245 - //@@todo intelligent unique id generation based on svn version of file (rather than just grabbing the $wgStyleVersion var)
246 - //@@todo we should check the packaged message text in this javascript file for updates and update the $mScriptLoaderURID id (in getJsClassFromPath)
247 -
248 - //generate the unique request param (combine with the most recent revision id of any wiki page with the $wgStyleVersion var)
249 -
250 - return Html::linkedScript( wfScript( 'mwScriptLoader' ) . "?class={$class_list}{$debug_param}&" . $this->getURIDparam() );
 244+ return Html::linkedScript( wfScript( 'mwScriptLoader' ) . "?class={$class_list}{$debug_param}&" . $this->getURIDparam( $classAry) );
251245 }
252246
253 - function getURIDparam(){
254 - global $wgDebugJavaScript, $wgStyleVersion;
 247+ function getURIDparam( $classAry=array() ){
 248+ global $wgDebugJavaScript, $wgStyleVersion, $IP, $wgScriptModifiedCheck;
255249 if( $wgDebugJavaScript ){
256250 return 'urid=' . time();
257251 } else {
258 - return "urid={$wgStyleVersion}_{$this->mLatestScriptRevID}";
 252+ $ftime=0;
 253+ if($wgScriptModifiedCheck){
 254+ foreach( $classAry as $class ){
 255+ $js_path = jsScriptLoader::getJsPathFromClass( $class );
 256+ if( $js_path ){
 257+ $cur_ftime = filemtime ( $IP ."/". $js_path );
 258+ if( $cur_ftime > $ftime )
 259+ $ftime = $cur_ftime;
 260+ }
 261+ }
 262+ }
 263+ //set up the urid:
 264+ $urid = "urid={$wgStyleVersion}";
 265+
 266+ //if we have a $this->mLatestScriptRevID (wiki page revision ids)
 267+ if($this->mLatestScriptRevID != 0 )
 268+ $urid .= "_{$this->mLatestScriptRevID}";
 269+
 270+ if( $ftime != 0 )
 271+ $urid .= "_".$ftime;
 272+
 273+ return $urid;
259274 }
260275 }
261276
Index: trunk/phase3/includes/api/ApiFormatJson.php
——@@ -67,24 +67,7 @@
6868 $prefix = preg_replace("/[^][.\\'\\\"_A-Za-z0-9]/", "", $callback ) . "(";
6969 $suffix = ")";
7070 }
71 -
72 - // Some versions of PHP have a broken json_encode, see PHP bug
73 - // 46944. Test encoding an affected character (U+20000) to
74 - // avoid this.
75 - $this->printText( $prefix . $this->getJsonEncode($this->getResultData(), $this->getIsHtml() ) . $suffix);
76 - }
77 - /*
78 - * static to support static calls to json output (instead of json_encode function)
79 - * @param array $results the results array to output as a json string
80 - * @parm isHTML if the output is html
81 - */
82 - public static function getJsonEncode($value, $isHtml=false){
83 - if (!function_exists('json_encode') || $isHtml || strtolower(json_encode("\xf0\xa0\x80\x80")) != '\ud840\udc00') {
84 - $json = new Services_JSON();
85 - return $json->encode($value, $isHtml) ;
86 - } else {
87 - return json_encode($value);
88 - }
 71+ $this->printText( $prefix . FormatJson::encode( $this->getResultData(), $this->getIsHtml() ) . $suffix);
8972 }
9073
9174 public function getAllowedParams() {
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -2743,6 +2743,18 @@
27442744 $wgEnableScriptLoader = false;
27452745
27462746 /*
 2747+ * $wgScriptModifiedCheck should run a file modified check on javascript files when
 2748+ * generating unique request ids for javascript include using the script-loader
 2749+ *
 2750+ * note this will only check core scripts that are directly included on the page.
 2751+ * (not scripts loaded after the initial page display since after initial page
 2752+ * display scripts inherit the unique request id)
 2753+ *
 2754+ * and or you can update $wgStyleVersion
 2755+ */
 2756+$wgScriptModifiedCheck = true;
 2757+
 2758+/*
27472759 * enable js2 Script System
27482760 * if enabled we include jquery, mv_embed and js2 versions of editPage.js
27492761 */

Follow-up revisions

RevisionCommit summaryAuthorDate
r56136more (bug 20336)...dale14:33, 10 September 2009

Comments

#Comment by Nikerabbit (talk | contribs)   05:39, 10 September 2009 
+class FormatJson{

Since it is a class it doesn't need to be in GlobalFunctions.php but can be autoloaded instead. 

* @param $forcClassAry Boolean: false by default
function getScriptLoaderJs( $classAry = array() ){

More docs for the parameter. 

+ * and or you can update $wgStyleVersion

and or -> or

#Comment by Catrope (talk | contribs)   11:56, 10 September 2009

In fact, there's no reason for FormatJson to be a class, since it only has two static functions, which could easily be renamed to wfEncodeJson() and wfDecodeJson()

#Comment by Nikerabbit (talk | contribs)   13:46, 10 September 2009 
3368 14449 93876 includes/GlobalFunctions.php

In my opinion GlobalFunctions.php is already big enough with over 3000 lines and almost 100 KiB in size. Classes are a natural way to split code into different files.

#Comment by 😂 (talk | contribs)   17:15, 10 September 2009

Sort of agree on this. I'm not really sure why we're not doing this like we do with our other compat functions (like iconv, etc) in GlobalFunctions. It's more confusing to have to memorize different function names as opposed to json_encode/json_decode.

#Comment by Nikerabbit (talk | contribs)   17:27, 10 September 2009

It is not possible to redefine functions in PHP, which would be needed to work around the PHP bug mentioned in comments, am I correct?

#Comment by Mdale (talk | contribs)   17:58, 10 September 2009

that is correct some version of php have bad json_encode support. But maybe versions of php with that bug are becoming rare I don't know?

#Comment by Mdale (talk | contribs)   14:34, 10 September 2009

oky...put it in a separate folder and moved the other json_service there to be non api specific. Added it to the AutoLoader in r56136

#Comment by Brion VIBBER (talk | contribs)   19:20, 10 September 2009

Decoder is broken for associative mode: 

> $code = json_encode( array( 'foo' => array( 'bar' => array( 'baz' => 'quux' ) ) ) );

> return $code;
{"foo":{"bar":{"baz":"quux"}}}

> $serv = new Services_JSON();
> return (array)$serv->decode($code);
array(1) {
  ["foo"]=>
  object(stdClass)#19 (1) {
    ["bar"]=>
    object(stdClass)#20 (1) {
      ["baz"]=>
      string(4) "quux"
    }
  }
}

Whereas I should get all arrays: 

> return json_decode($code, true);
array(1) {
  ["foo"]=>
  array(1) {
    ["bar"]=>
    array(1) {
      ["baz"]=>
      string(4) "quux"
    }
  }
}
#Comment by Brion VIBBER (talk | contribs)   19:33, 10 September 2009

Fixed in r56146, thanks!

Status & tagging log