r87326 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r87325‎ | r87326 | r87327 >
Date:13:18, 3 May 2011
Author:pcopp
Status:ok (Comments)
Tags:
Comment:
Fix Bug 28354: Edit tab is shown as "view source" for blocked users, which breaks squid caching

* Skip user block checks for Title::quickUserCan(). This restores the behavior from 1.16 rsp. before r65504.
* Remove unnecessary check for "$short && count($errors)", this is already handled by getUserPermissionsErrorsInternal().
Modified paths:
  • /trunk/phase3/includes/Title.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Title.php
——@@ -1175,7 +1175,8 @@
11761176 *
11771177 * @param $action String action that permission needs to be checked for
11781178 * @param $user User to check
1179 - * @param $doExpensiveQueries Bool Set this to false to avoid doing unnecessary queries.
 1179+ * @param $doExpensiveQueries Bool Set this to false to avoid doing unnecessary queries by
 1180+ * skipping checks for cascading protections and user blocks.
11801181 * @param $ignoreErrors Array of Strings Set this to a list of message keys whose corresponding errors may be ignored.
11811182 * @return Array of arguments to wfMsg to explain permissions problems.
11821183 */
——@@ -1521,7 +1522,7 @@
15221523 * @return Array list of errors
15231524 */
15241525 private function checkUserBlock( $action, $user, $errors, $doExpensiveQueries, $short ) {
1525 - if( $short && count( $errors ) > 0 ) {
 1526+ if( !$doExpensiveQueries ) {
15261527 return $errors;
15271528 }
15281529

Follow-up revisions

RevisionCommit summaryAuthorDate
r87395Follow-up r87326: Add regression test.pcopp11:51, 4 May 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r65504* Refactor super-long title function...mah06:22, 24 April 2010

Comments

#Comment by MarkAHershberger (talk | contribs)   15:32, 3 May 2011

Thanks for finding this and fixing it. Do you think you could add/update the tests from my revision that seems to have caused the problem?

#Comment by P.Copp (talk | contribs)   11:54, 4 May 2011

Done in r87395.

#Comment by Brion VIBBER (talk | contribs)   18:26, 7 June 2011

This removes handling of the $short parameter. Is this intended? What differences in behavior are expected with this parameter on/off?

#Comment by P.Copp (talk | contribs)   21:52, 7 June 2011

Yes, I just removed the check as it is duplicated in Title::getUserPermissionsErrorsInternal(): 

            while( count( $checks ) > 0 &&
	                           !( $short && count( $errors ) > 0 ) ) {
	                        $method = array_shift( $checks );
	                        $errors = $this->$method( $action, $user, $errors, $doExpensiveQueries, $short );
            }

The parameter $short is merely a residue of the refactoring of r65504 and could be removed from most of these functions.

#Comment by Brion VIBBER (talk | contribs)   22:27, 7 June 2011

I'll leave that as an exercise for future cleanup then. :)

No clear problems standing out otherwise.

Status & tagging log