r111017 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r111016‎ | r111017 | r111018 >
Date:01:34, 9 February 2012
Author:jeroendedauw
Status:reverted (Comments)
Tags:core 
Comment:
put in r110285 again now that 1.19 branched
Modified paths:
  • /trunk/phase3/includes/Title.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Title.php
——@@ -4164,7 +4164,21 @@
41654165 * @return Bool
41664166 */
41674167 public function isKnown() {
4168 - return $this->isAlwaysKnown() || $this->exists();
 4168+ $isKnown = null;
 4169+
 4170+ /**
 4171+ * Allows overriding default behaviour for determining if a page exists.
 4172+ * If $isKnown is kept as null, regular checks happen. If it's
 4173+ * a boolean, this value is returned by the isKnown method.
 4174+ *
 4175+ * @since 1.20
 4176+ *
 4177+ * @param Title $title
 4178+ * @param boolean|null $isKnown
 4179+ */
 4180+ wfRunHooks( 'TitleIsKnown', array( $this, &$isKnown ) );
 4181+
 4182+ return is_null( $isKnown ) ? ( $this->isAlwaysKnown() || $this->exists() ) : $isKnown;
41694183 }
41704184
41714185 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r111033follow up to r111017 - added docsjeroendedauw14:10, 9 February 2012
r111069follow up to r111017 - up rel notesjeroendedauw19:14, 9 February 2012
r111335follow up to r111017, move hook to isAlwatsKnown as per discussion on CRjeroendedauw23:10, 12 February 2012
r111338add warning note to isKnown() per r111017robin23:42, 12 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r110285add hook that allows changing the check to see if a page exists or notjeroendedauw12:52, 30 January 2012

Comments

#Comment by Nikerabbit (talk | contribs)   08:03, 9 February 2012

docs/hooks.txt?

#Comment by Siebrand (talk | contribs)   19:03, 9 February 2012

RELEASE-NOTES update missing.

#Comment by SPQRobin (talk | contribs)   16:25, 11 February 2012

This is useful, I was considering adding such a hook too. However, this doesn't affect internal links in page content (they remain redlinks) and I think it would be better/more logical if it does. They're checked in LinkHolderArray.php calling Title->isAlwaysKnown() and page existence checks are optimised in that file so I suppose we can't just change it to call isKnown(). We could perhaps either add a parameter to isKnown() which would skip the $this->exists() check or move this hook to isAlwaysKnown().

#Comment by Jeroen De Dauw (talk | contribs)   21:01, 11 February 2012

Good, catch, I had actually not noticed links in pages remained redlinks. That sucks.

I'm reluctant to just move over the hook though, as the docs for isAlwaysKnown state:

"This function is semi-deprecated for public use, as well as somewhat misleadingly named. You probably just want to call isKnown(), which calls this function internally."

What about having LinkHolderArray call isKnown instead? Would this cause issues?

#Comment by SPQRobin (talk | contribs)   22:39, 12 February 2012

isKnown was introduced in r44520, with the intention to be a shortcut to both check page existence and to isAlwaysKnown. LinkHolderArray calls isAlwaysKnown and does the page existence check separately so it can do them in batches for pages with a lot of links. Consequently, to ensure consistency, it's best that isKnown remains a "shortcut" exclusively to exists() and isAlwaysKnown. It may be that other functions only call isAlwaysKnown and do their own page existence check.

So I would move the hook to isAlwaysKnown, what the docs say is intended for functions calling it, while the hook we want to add is for extra exceptions similar to those in isAlwaysKnown.

#Comment by Jeroen De Dauw (talk | contribs)   23:04, 12 February 2012

In that case I will move the hook as suggested. So what's the deal with the note of "semi deprecation" for isAlwaysKnown? Let's kill that?

#Comment by SPQRobin (talk | contribs)   23:44, 12 February 2012

Thanks for moving.

That note might be removed, i think it just means that most calls should be to isKnown. Not really important I think.

In any case I added a note in r111338 about isKnown().

Status & tagging log