r73875 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r73874‎ | r73875 | r73876 >
Date:08:16, 28 September 2010
Author:nikerabbit
Status:ok (Comments)
Tags:brion 
Comment:
Don't show empty source code for non-existing pages if editing was prevented by a permission error
Modified paths:
  • /trunk/phase3/includes/EditPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/EditPage.php
——@@ -330,7 +330,9 @@
331331 $permErrors = $this->getEditPermissionErrors();
332332 if ( $permErrors ) {
333333 wfDebug( __METHOD__ . ": User can't edit\n" );
334 - $this->readOnlyPage( $this->getContent( false ), true, $permErrors, 'edit' );
 334+ $content = $this->getContent( null );
 335+ $content = $content === '' ? null : $content;
 336+ $this->readOnlyPage( $content, true, $permErrors, 'edit' );
335337 wfProfileOut( __METHOD__ );
336338 return;
337339 } else {
——@@ -1173,7 +1175,7 @@
11741176 * parameter; will be called during form output
11751177 * near the top, for captchas and the like.
11761178 */
1177 - function showEditForm( $formCallback=null ) {
 1179+ function showEditForm( $formCallback = null ) {
11781180 global $wgOut, $wgUser, $wgTitle;
11791181
11801182 # If $wgTitle is null, that means we're in API mode.

Comments

#Comment by Brion VIBBER (talk | contribs)   15:03, 28 September 2010

It looks like this bit: 

    $content = $content === '' ? null : $content;

Will pass the null down to OutputPage::readOnlyPage(), causing the textarea not to be shown, if the page *does* exist but is empty; this seems a little flaky. Is that right?

#Comment by Nikerabbit (talk | contribs)   15:53, 28 September 2010

Yes it is. But isn't this better than displaying empty box when the page doesn't even exists? I don't fancy making changes to Article::getContent which is full of side effects.

#Comment by Brion VIBBER (talk | contribs)   15:54, 28 September 2010

Hmm, probably we could check if the article exists, then only if it does we can go ahead and fetch its content. This should avoid having to worry about the Horror that is getContent() in the funky case ;)

#Comment by Nikerabbit (talk | contribs)   16:02, 28 September 2010

But that would change behavior, because pages which do not exists can still have content according to Article::getContent().

#Comment by Brion VIBBER (talk | contribs)   19:40, 30 January 2011

I believe my recommendation was meant to be to add that logic around the call to $this->readOnlyPage().

#Comment by Siebrand (talk | contribs)   15:38, 23 October 2010

Brion, can you revisit this discussion, please?

#Comment by Siebrand (talk | contribs)   19:36, 30 January 2011

/me pokes Brion a second time.

#Comment by Nikerabbit (talk | contribs)   08:17, 2 April 2011

Since nobody has complained about this edge case (which imho makes sense to me) I think this is fine.

Status & tagging log