r101149 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r101148‎ | r101149 | r101150 >
Date:16:39, 28 October 2011
Author:ckepper
Status:ok (Comments)
Tags:
Comment:
fixed bug 31613 (API UsageException when saving a collection to a page)
Modified paths:
  • /trunk/extensions/Collection/Collection.body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Collection/Collection.body.php
——@@ -863,7 +863,7 @@
864864 'title' => $title->getPrefixedText(),
865865 'text' => $articleText,
866866 'token' => $wgUser->editToken(),
867 - ), true );
 867+ ), true, $_SESSION );
868868 $api = new ApiMain( $req, true );
869869 $api->execute();
870870 return true;

Sign-offs

UserFlagDate
Bryaninspected20:34, 9 November 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r102949Followup r101149: make FauxRequest::__construct() use $_SESSION if the sessio...catrope08:40, 14 November 2011
r1029501.18wmf1: MFT r101149catrope08:41, 14 November 2011
r113990MFT r101149reedy01:41, 16 March 2012

Comments

#Comment by 😂 (talk | contribs)   21:51, 8 November 2011

Rather than passing the whole $_SESSION to FauxRequest, how about just constructing an array with the variables you need?

#Comment by Catrope (talk | contribs)   17:53, 9 November 2011

I was hoping we could fix this in a better way, by either

  1. Having ApiMain pull session data from $_SESSION instead of the request object, or
  2. having FauxRequest use $_SESSION instead of its own session data, or
  3. having the FauxRequest constructor populate the request object's session data from $_SESSION if the third parameter isn't given

Thoughts?

#Comment by Bryan (talk | contribs)   20:34, 9 November 2011

Of those three, only 3 is could be used imho. The other ones smell like globals. I find the current solution acceptable.

#Comment by Catrope (talk | contribs)   08:40, 14 November 2011

Did #3 in r102949.

#Comment by Catrope (talk | contribs)   09:24, 14 November 2011

...and reverted. But none of that is needed now that we have DerivativeRequest, which should be used instead of FauxRequest in 1.19+.

#Comment by Ckepper (talk | contribs)   18:10, 9 November 2011

In my opinion this is just a quick fix. A better way to solve this issue was suggested by Daniel Friesen. This would probably result in a major refactoring of the method and so I shied away from it.

#Comment by G.Hagedorn (talk | contribs)   21:56, 19 November 2011

MAH removed 1.18 tag Nov. 7. Fixes bug 31613, is live on 1.18WMF1. Please consider backporting to 1.18.

Status & tagging log