Skip to content

Sitelink refactor #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 2, 2024
Merged

Sitelink refactor #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0070a5
Add ItemPageLookupFactory
malberts Nov 30, 2024
4b96d54
Update test
malberts Nov 30, 2024
827b243
Refactor SiteLinkItemPageLookup
malberts Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/Persistence/ItemPageLookupFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

declare( strict_types = 1 );

namespace ProfessionalWiki\WikibaseFacetedSearch\Persistence;

use ProfessionalWiki\WikibaseFacetedSearch\Application\Config;
use Wikibase\Repo\WikibaseRepo;

class ItemPageLookupFactory {

public function __construct(
private readonly Config $config
) {
}

public function newItemPageLookup(): ItemPageLookup {
if ( $this->config->linkTargetSitelinkSiteId !== null ) {
return new SiteLinkItemPageLookup(
WikibaseRepo::getStore()->newSiteLinkStore(),
$this->config->linkTargetSitelinkSiteId
);
}

return new NullItemPageLookup();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What motivated you to put this factory logic into a dedicated class? (I'm not objecting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

16 changes: 16 additions & 0 deletions src/Persistence/NullItemPageLookup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

declare( strict_types = 1 );

namespace ProfessionalWiki\WikibaseFacetedSearch\Persistence;

use Title;
use Wikibase\DataModel\Entity\ItemId;

class NullItemPageLookup implements ItemPageLookup {

public function getPageTitle( ItemId $itemId ): ?Title {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case, this code could go into its own method:

$pageTitle = self::getItemPage( $itemId );

		if ( $pageTitle === null ) {
			return;
		}

		$title = $pageTitle;
		```

Copy link
Contributor Author

@malberts malberts Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


}
16 changes: 12 additions & 4 deletions src/Persistence/SiteLinkItemPageLookup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use Title;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\SiteLink;
use Wikibase\Lib\Store\SiteLinkLookup;

class SiteLinkItemPageLookup implements ItemPageLookup {
Expand All @@ -17,10 +18,7 @@ public function __construct(
}

public function getPageTitle( ItemId $itemId ): ?Title {
$siteLink = array_filter(
$this->siteLinksStore->getSiteLinksForItem( $itemId ),
fn( $siteLink ) => $siteLink->getSiteId() === $this->siteLinkSiteId
)[0] ?? null;
$siteLink = $this->getConfiguredSiteLink( $this->siteLinksStore->getSiteLinksForItem( $itemId ) );

if ( $siteLink === null ) {
return null;
Expand All @@ -29,4 +27,14 @@ public function getPageTitle( ItemId $itemId ): ?Title {
return Title::newFromText( $siteLink->getPageName() );
}

/**
* @param SiteLink[] $siteLinks
*/
private function getConfiguredSiteLink( array $siteLinks ): ?SiteLink {
return array_filter(
$siteLinks,
fn( $siteLink ) => $siteLink->getSiteId() === $this->siteLinkSiteId
)[0] ?? null;
}

}
12 changes: 7 additions & 5 deletions src/WikibaseFacetedSearchExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\ConfigJsonValidator;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\ConfigLookup;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\ItemPageLookup;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\ItemPageLookupFactory;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\PageContentConfigLookup;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\PageContentFetcher;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\SiteLinkItemPageLookup;
use Title;
use Wikibase\Repo\WikibaseRepo;

class WikibaseFacetedSearchExtension {

Expand All @@ -31,9 +30,12 @@ public static function getInstance(): self {
}

public function getItemPageLookup(): ItemPageLookup {
return new SiteLinkItemPageLookup(
WikibaseRepo::getStore()->newSiteLinkStore(),
$this->getConfig()->linkTargetSitelinkSiteId ?? ''
return $this->newItemPageLookupFactory()->newItemPageLookup();
}

private function newItemPageLookupFactory(): ItemPageLookupFactory {
return new ItemPageLookupFactory(
$this->getConfig()
);
}

Expand Down
36 changes: 36 additions & 0 deletions tests/Persistence/ItemPageLookupFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

declare( strict_types = 1 );

namespace ProfessionalWiki\WikibaseFacetedSearch\Tests\Persistence;

use PHPUnit\Framework\TestCase;
use ProfessionalWiki\WikibaseFacetedSearch\Application\Config;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\ItemPageLookupFactory;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\NullItemPageLookup;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\SiteLinkItemPageLookup;

/**
* @covers \ProfessionalWiki\WikibaseFacetedSearch\Persistence\ItemPageLookupFactory
*/
class ItemPageLookupFactoryTest extends TestCase {

public function testReturnsNullItemPageLookupWhenNoSiteIdIsConfigured(): void {
$factory = new ItemPageLookupFactory(
new Config()
);

$this->assertInstanceOf( NullItemPageLookup::class, $factory->newItemPageLookup() );
}

public function testReturnsSiteLinkItemPageLookupWhenSiteIdIsConfigured(): void {
$factory = new ItemPageLookupFactory(
new Config(
linkTargetSitelinkSiteId: 'enwiki'
)
);

$this->assertInstanceOf( SiteLinkItemPageLookup::class, $factory->newItemPageLookup() );
}

}
22 changes: 22 additions & 0 deletions tests/Persistence/NullItemPageLookupTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

declare( strict_types = 1 );

namespace ProfessionalWiki\WikibaseFacetedSearch\Tests\Persistence;

use PHPUnit\Framework\TestCase;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\NullItemPageLookup;
use Wikibase\DataModel\Entity\ItemId;

/**
* @covers \ProfessionalWiki\WikibaseFacetedSearch\Persistence\NullItemPageLookup
*/
class NullItemPageLookupTest extends TestCase {

public function testReturnsNull(): void {
$this->assertNull(
( new NullItemPageLookup() )->getPageTitle( new ItemId( 'Q42' ) )
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone wants their 100% code coverage :D

}

}
2 changes: 1 addition & 1 deletion tests/Persistence/SiteLinkItemPageLookupTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ public function testReturnsNullWhenOnlyOtherSiteLinksExist(): void {
}

public function testReturnsPageWhenManySiteLinksExist(): void {
$this->createItemWithSiteLink( 'Q42', self::SITE_ID, 'Page for Q42' );
$this->createItemWithSiteLink( 'Q100', 'otherSiteId', 'Other page' );
$this->createItemWithSiteLink( 'Q42', self::SITE_ID, 'Page for Q42' );
$this->createItemWithSiteLink( 'Q200', 'anotherSiteId', 'Another page' );

$this->assertItemPageHasTitle( 'Q42', 'Page for Q42' );
Expand Down