Sitelink refactor #26
Conversation
malberts
commented
Nov 30, 2024
malberts
commented
- Add ItemPageLookupFactory to allow for non-sitelink page lookups.
- Rewrite item link with page sitelink #24 (comment)
- Rewrite item link with page sitelink #24 (comment)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26 +/- ##
============================================
+ Coverage 28.97% 32.43% +3.45%
- Complexity 79 85 +6
============================================
Files 12 14 +2
Lines 245 259 +14
============================================
+ Hits 71 84 +13
- Misses 174 175 +1 ☔ View full report in Codecov by Sentry. |
malberts
force-pushed
the
sitelink-refactor
branch
from
f155439 to
827b243
Compare
| public function testReturnsNull(): void { | ||
| $this->assertNull( | ||
| ( new NullItemPageLookup() )->getPageTitle( new ItemId( 'Q42' ) ) | ||
| ); |
There was a problem hiding this comment.
Someone wants their 100% code coverage :D
|
|
||
| return new NullItemPageLookup(); | ||
| } | ||
| } |
There was a problem hiding this comment.
What motivated you to put this factory logic into a dedicated class? (I'm not objecting)
There was a problem hiding this comment.
I figured it would be slightly more testable than being directly in the extension factory (where we don't typically expect, and test, logic). But then again it's also slightly YAGNI since there are no other Lookups at the moment.
|
|
||
| public function getPageTitle( ItemId $itemId ): ?Title { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I wonder if the call to rewriteLinkForNonEntityResult in the hooks file is being skipped over in cases it should not when we always return null.
There was a problem hiding this comment.
In which case, this code could go into its own method:
$pageTitle = self::getItemPage( $itemId );
if ( $pageTitle === null ) {
return;
}
$title = $pageTitle;
```There was a problem hiding this comment.
rewriteLinkForNonEntityResult basically reimplements this: https://github.com/wikimedia/mediawiki-extensions-Wikibase/blob/3f636e441cf070234bfcd4f217603d2e2a0be28a/repo/includes/Hooks/HtmlPageLinkRendererEndHookHandler.php#L321-L327
When you override $title without $titleSnippet, MW ends up using $title's text for the link. So to get the original Item link text back, we need to trigger the original link text building and put that into $titleSnippet. I couldn't get it to work by manually triggering the entire hook that calls that code.
In the end if $pageTitle is null, then we shouldn't do anything further, because the original item link would already be correct as per the link building of Wikibase/WikibaseCirrusSearch.
