r69906 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r69905‎ | r69906 | r69907 >
Date:20:52, 25 July 2010
Author:reedy
Status:ok (Comments)
Tags:
Comment:
Article:: to self::
Modified paths:
  • /trunk/phase3/includes/CategoryPage.php (modified) (history)
  • /trunk/phase3/includes/ImagePage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/CategoryPage.php
@@ -18,7 +18,7 @@
1919 $diffOnly = $wgRequest->getBool( 'diffonly', $wgUser->getOption( 'diffonly' ) );
2020
2121 if ( isset( $diff ) && $diffOnly )
22 - return Article::view();
 22+ return parent::view();
2323
2424 if ( !wfRunHooks( 'CategoryPageView', array( &$this ) ) )
2525 return;
@@ -27,7 +27,7 @@
2828 $this->openShowCategory();
2929 }
3030
31 - Article::view();
 31+ parent::view();
3232
3333 if ( NS_CATEGORY == $this->mTitle->getNamespace() ) {
3434 $this->closeShowCategory();
Index: trunk/phase3/includes/ImagePage.php
@@ -66,7 +66,7 @@
6767 $diffOnly = $wgRequest->getBool( 'diffonly', $wgUser->getOption( 'diffonly' ) );
6868
6969 if ( $this->mTitle->getNamespace() != NS_FILE || ( isset( $diff ) && $diffOnly ) ) {
70 - return Article::view();
 70+ return parent::view();
7171 }
7272
7373 $this->loadFile();
@@ -76,7 +76,7 @@
7777 // mTitle is the same as the redirect target so ask Article
7878 // to perform the redirect for us.
7979 $wgRequest->setVal( 'diffonly', 'true' );
80 - return Article::view();
 80+ return parent::view();
8181 } else {
8282 // mTitle is not the same as the redirect target so it is
8383 // probably the redirect page itself. Fake the redirect symbol
@@ -106,7 +106,7 @@
107107
108108 # No need to display noarticletext, we use our own message, output in openShowImage()
109109 if ( $this->getID() ) {
110 - Article::view();
 110+ parent::view();
111111 } else {
112112 # Just need to set the right headers
113113 $wgOut->setArticleFlag( true );

Comments

#Comment by Simetrical (talk | contribs)   20:40, 28 July 2010

What's the goal of this? Isn't it somewhat more confusing, if anything? Offhand, I wouldn't know that CategoryPage and ImagePage inherit from Article, so the new version is less intuitive to me.

#Comment by Simetrical (talk | contribs)   20:45, 28 July 2010

Similar objection applies to r69908, r69910, r69911, r69914.

#Comment by Simetrical (talk | contribs)   21:23, 28 July 2010

And r69941. I should point out that I've more than once seen bugs caused when someone copy-pasted code using "self::" to another class and forgot to change it to refer to the actual class name. Although the reverse sort of bug is probably possible too.

#Comment by Reedy (talk | contribs)   07:01, 29 July 2010

Well, technically it should be $this->view() (well, something like). But that's redefined in the class (the method it's calling it in)

Also, view is not a static method in Article. So, calling Article::view(), is wrong.

You can't do $parent->view(), the syntax is parent::view()...

Does this resolve it for you?

The cases where it's changed, I think for all those revisions, that's the case...

#Comment by Simetrical (talk | contribs)   18:33, 29 July 2010

If that's the case, I guess it's fine, so I'm resetting to new.

#Comment by Reedy (talk | contribs)   07:06, 29 July 2010

Maybe the "problem" is not a specific/descriptive enough commit summary? ie "Why I am doing this?"

#Comment by MZMcBride (talk | contribs)   07:16, 29 July 2010

Well, poor commit summaries aren't a "problem," they're a problem. Other developers have to be able to figure out what you're doing and why in order to assess the code you write. Commit summaries are a key part of this process.

#Comment by Nikerabbit (talk | contribs)   15:53, 29 July 2010

Could we write short summary for how to write commit messages and place it somewhere which commiters are supposed to read (they might not, but at least we have a some place to refer them to).

Something like this:

  • Say what you did and why it's a good thing or needed.
  • The first line should contain concise summary of the commit.
  • In the following lines you can go into details of what and why.
  • Reference related bugs with bug 11111 and revisions with r69906 for each bug and revision.
#Comment by Hashar (talk | contribs)   13:16, 20 November 2010

What do we do with this revision ? Is it ok or should it be reverted as well as r69908, r69910, r69911, r69914 ?

#Comment by Reedy (talk | contribs)   00:36, 21 November 2010

A couple have been marked ok etc...

The revision isn't wrong, just my summaries were a bit crappy

Status & tagging log