r89408 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r89407‎ | r89408 | r89409 >
Date:11:04, 3 June 2011
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
More unpicking of r85288. I think this is all of the magic method calls, but they're very hard to grep for (part of the problem with them!), so let's leave the calls in with a wfDeprecated() for a while...
Modified paths:
  • /trunk/phase3/includes/RequestContext.php (modified) (history)
  • /trunk/phase3/includes/SpecialPage.php (modified) (history)
  • /trunk/phase3/includes/actions/DeleteAction.php (modified) (history)
  • /trunk/phase3/includes/api/ApiParse.php (modified) (history)
  • /trunk/phase3/maintenance/rebuildFileCache.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/parser/NewParserTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/suites/UploadFromUrlTestSuite.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/rebuildFileCache.php
——@@ -79,7 +79,7 @@
8080 $this->output( "Page {$row->page_id} has bad title\n" );
8181 continue; // broken title?
8282 }
83 - $wgOut = $context->output; // set display title
 83+ $wgOut = $context->getOutput(); // set display title
8484 $article = new Article( $wgTitle );
8585 // If the article is cacheable, then load it
8686 if ( $article->isFileCacheable() ) {
Index: trunk/phase3/tests/phpunit/includes/parser/NewParserTest.php
——@@ -79,8 +79,8 @@
8080 // $tmpGlobals['wgContLang'] = new StubContLang;
8181 $tmpGlobals['wgUser'] = new User;
8282 $context = new RequestContext();
83 - $tmpGlobals['wgLang'] = $context->lang;
84 - $tmpGlobals['wgOut'] = $context->output;
 83+ $tmpGlobals['wgLang'] = $context->getLang();
 84+ $tmpGlobals['wgOut'] = $context->getOutput();
8585 $tmpGlobals['wgParser'] = new StubObject( 'wgParser', $GLOBALS['wgParserConf']['class'], array( $GLOBALS['wgParserConf'] ) );
8686 $tmpGlobals['wgRequest'] = new WebRequest;
8787
——@@ -312,10 +312,10 @@
313313 $langObj = Language::factory( $lang );
314314 $GLOBALS['wgContLang'] = $langObj;
315315 $context = new RequestContext();
316 - $GLOBALS['wgLang'] = $context->lang;
 316+ $GLOBALS['wgLang'] = $context->getLang();
317317
318318 $GLOBALS['wgMemc'] = new EmptyBagOStuff;
319 - $GLOBALS['wgOut'] = $context->output;
 319+ $GLOBALS['wgOut'] = $context->getOutput();
320320
321321 global $wgHooks;
322322
Index: trunk/phase3/tests/phpunit/suites/UploadFromUrlTestSuite.php
——@@ -49,8 +49,8 @@
5050 // $wgContLang = new StubContLang;
5151 $wgUser = new User;
5252 $context = new RequestContext;
53 - $wgLang = $context->lang;
54 - $wgOut = $context->output;
 53+ $wgLang = $context->getLang();
 54+ $wgOut = $context->getOutput();
5555 $wgParser = new StubObject( 'wgParser', $wgParserConf['class'], array( $wgParserConf ) );
5656 $wgRequest = new WebRequest;
5757
Index: trunk/phase3/includes/RequestContext.php
——@@ -219,6 +219,7 @@
220220 * @return string
221221 */
222222 public function __get( $name ) {
 223+ wfDeprecated( 'RequestContext::__get() is deprecated; use $context->getFoo() instead' );
223224 if ( in_array( $name, array( 'request', 'title', 'output', 'user', 'lang', 'skin' ) ) ) {
224225 $fname = 'get' . ucfirst( $name );
225226 return $this->$fname();
——@@ -232,6 +233,7 @@
233234 * @return string
234235 */
235236 public function __set( $name, $value ) {
 237+ wfDeprecated( 'RequestContext::__set() is deprecated; use $context->setFoo() instead' );
236238 if ( in_array( $name, array( 'request', 'title', 'output', 'user', 'lang', 'skin' ) ) ) {
237239 $fname = 'set' . ucfirst( $name );
238240 return $this->$fname( $value );
Index: trunk/phase3/includes/actions/DeleteAction.php
——@@ -236,7 +236,7 @@
237237 $data['Reason'] = (array)$data['Reason'];
238238
239239 $error = null;
240 - if ( !wfRunHooks( 'ArticleDelete', array( &$page, &$context->user, &$data['Reason'][0], &$error ) ) ) {
 240+ if ( !wfRunHooks( 'ArticleDelete', array( &$page, $context->getUser(), &$data['Reason'][0], &$error ) ) ) {
241241 return $error;
242242 }
243243
——@@ -376,7 +376,7 @@
377377 $dbw->commit();
378378 }
379379
380 - wfRunHooks( 'ArticleDeleteComplete', array( &$page, &$context->user, $data['Reason'][0], $id ) );
 380+ wfRunHooks( 'ArticleDeleteComplete', array( &$page, $context->getUser(), $data['Reason'][0], $id ) );
381381 return true;
382382 }
383383
Index: trunk/phase3/includes/api/ApiParse.php
——@@ -244,22 +244,22 @@
245245
246246 if ( isset( $prop['headitems'] ) || isset( $prop['headhtml'] ) ) {
247247 $context = new RequestContext;
248 - $context->output->addParserOutputNoText( $p_result );
 248+ $context->getOutput()->addParserOutputNoText( $p_result );
249249
250250 if ( isset( $prop['headitems'] ) ) {
251251 $headItems = $this->formatHeadItems( $p_result->getHeadItems() );
252252
253 - $context->skin->setupUserCss( $context->output );
254 - $css = $this->formatCss( $context->output->buildCssLinksArray() );
 253+ $context->getSkin()->setupUserCss( $context->getOutput() );
 254+ $css = $this->formatCss( $context->getOutput()->buildCssLinksArray() );
255255
256 - $scripts = array( $context->output->getHeadScripts( $context->skin ) );
 256+ $scripts = array( $context->getOutput()->getHeadScripts( $context->getSkin() ) );
257257
258258 $result_array['headitems'] = array_merge( $headItems, $css, $scripts );
259259 }
260260
261261 if ( isset( $prop['headhtml'] ) ) {
262262 $result_array['headhtml'] = array();
263 - $result->setContent( $result_array['headhtml'], $context->output->headElement( $context->skin ) );
 263+ $result->setContent( $result_array['headhtml'], $context->getOutput()->headElement( $context->getSkin() ) );
264264 }
265265 }
266266
Index: trunk/phase3/includes/SpecialPage.php
——@@ -873,8 +873,8 @@
874874 $params = array();
875875
876876 foreach( $this->mAllowedRedirectParams as $arg ) {
877 - if( $this->getContext()->request->getVal( $arg, null ) !== null ){
878 - $params[$arg] = $this->getContext()->request->getVal( $arg );
 877+ if( $this->getRequest()->getVal( $arg, null ) !== null ){
 878+ $params[$arg] = $this->getRequest()->getVal( $arg );
879879 }
880880 }
881881

Follow-up revisions

RevisionCommit summaryAuthorDate
r89474Follow up r89408. Magic >lang to getLang().platonides20:12, 4 June 2011
r90901Follow-up r89408, r86872: restore IContextSource and ContextSource, to be mor...happy-melon19:38, 27 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r85288Implement magic accessors for RequestContext variables: you can now just call...happy-melon22:09, 3 April 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   22:20, 3 June 2011

This has changed a number of hook parameter calls from pass-by-reference to pass-by-object: 

-		if ( !wfRunHooks( 'ArticleDelete', array( &$page, &$context->user, &$data['Reason'][0], &$error ) ) ) {
+		if ( !wfRunHooks( 'ArticleDelete', array( &$page, $context->getUser(), &$data['Reason'][0], &$error ) ) ) {

hooks.txt unfortunately doesn't list any of these parameters here as being references (grrrr!) but callees may need to be fixed etc. If fixing these, it's probably an appropriate time to also remove the other unnecessary references: there the $page and $user params should *not* be references (as they cannot be swapped out for another object); the $reason parameter.... probably? shouldn't be editable. Here, only the $error param actually needs to be passed by reference, so the hook can replace it.

#Comment by Platonides (talk | contribs)   18:23, 4 June 2011

We can't really change the way the hook parameters are passed. We should revert this change.

#Comment by Happy-melon (talk | contribs)   18:38, 4 June 2011

Hooks can still do things like $user->setSomething() and that will stick; it's just that they can't do $user = $otherUser. Which as Brion says is not kosher anyway. Changing it to be truly read-only would indeed not be feasible; but changing from pass-by-reference to pass-by-object is ok, IMO.

#Comment by Platonides (talk | contribs)   20:14, 4 June 2011

Can they, or will old extensions hooking it fail to run because they were expecting a reference and it is now given by value?

That's the reason we didn't fix all prototypes everywhere long time ago.

#Comment by Aaron Schulz (talk | contribs)   02:13, 5 August 2011

Can any hooks just be updated. Is this rev OK now?

#Comment by Nikerabbit (talk | contribs)   14:37, 4 June 2011

Notice: Use of RequestContext::__get() is deprecated; use $context->getFoo() instead is deprecated. [Called from StubUserLang::_newObject in /www/sandwiki/includes/StubObject.php at line 151] in /www/sandwiki/includes/GlobalFunctions.php on line 3342

Notice: Use of RequestContext::__set() is deprecated; use $context->setFoo() instead is deprecated. [Called from SpecialPageFactory::executePath in /www/sandwiki/includes/SpecialPageFactory.php at line 447] in /www/sandwiki/includes/GlobalFunctions.php on line 3342

#Comment by Platonides (talk | contribs)   18:24, 4 June 2011

The point is, StubObject::_newObject is doing RequestContext::getMain()->lang; so it shouldn't have triggered __get(). Either php or our expectations are wrong.

#Comment by Happy-melon (talk | contribs)   18:39, 4 June 2011

Yes it should; RequestContext::getMain()->lang is equivalent to $context->lang; the __get() magic accessor is to rewrite that to $context->getLang().

#Comment by Platonides (talk | contribs)   20:12, 4 June 2011

Oops. You are right. I was looking just at the RequestContext::getMain() Fixed that at r89474.

#Comment by Nikerabbit (talk | contribs)   07:50, 5 June 2011

Notice: Use of RequestContext::__get() is deprecated; use $context->getFoo() instead is deprecated. [Called from MediaWiki::parseTitle in /www/sandwiki/includes/Wiki.php at line 36] in /www/sandwiki/includes/GlobalFunctions.php on line 3342

#Comment by 😂 (talk | contribs)   07:59, 5 June 2011

Why on earth are we adding and deprecating this within the same release?

Just remove the __get() magic entirely so you through an E_FATAL immediately rather than this sneaking about with notices that may go...unnoticed.

#Comment by Nikerabbit (talk | contribs)   08:05, 5 June 2011

Shouldn't go unnoticed. Every developer should enable and log all warnings :)

#Comment by 😂 (talk | contribs)   08:13, 5 June 2011

I do, you do, and most smart developers do :) My point remains though: why do we even need to keep the __get() thing in the same release? It was a design decision during the iterations on RequestContext and we shouldn't keep it around (lest people *start* using it!)

#Comment by Nikerabbit (talk | contribs)   08:15, 5 June 2011

It's nicer to keep the trunk running until most of them are fixed.

#Comment by 😂 (talk | contribs)   08:16, 5 June 2011

Fair 'nuff.

#Comment by Happy-melon (talk | contribs)   08:38, 5 June 2011

This.

#Comment by Nikerabbit (talk | contribs)   10:13, 17 June 2011

Wiki.php is full of __get __set abuse. 

tail -n 5000 logs/error_php | grep Notice | cut -d" " -f 3- | sort | uniq -c | sort -n | grep deprecated
     83 PHP Notice:  Use of RequestContext::__set() is deprecated; use $context->setFoo() instead is deprecated. [Called from MediaWiki::initializeArticle in /www/w/includes/Wiki.php at line 304] in /www/w/includes/GlobalFunctions.php on line 3342
    170 PHP Notice:  Use of RequestContext::__get() is deprecated; use $context->getFoo() instead is deprecated. [Called from MediaWiki::parseTitle in /www/w/includes/Wiki.php at line 36] in /www/w/includes/GlobalFunctions.php on line 3342

First one already reported almost two weeks ago.

#Comment by Nikerabbit (talk | contribs)   12:55, 17 June 2011

Seems like Reedy fixed them. Thanks!

#Comment by Happy-melon (talk | contribs)   12:54, 27 June 2011

Is anyone still seeing these?

#Comment by Reedy (talk | contribs)   23:02, 15 July 2011

I'm guessing there's a few more followups that need dealing with...

Status & tagging log