Skip to content
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.

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

Rewrite item link with page sitelink #24

Merged
merged 1 commit into from
Nov 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 4 additions & 46 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
@@ -1,55 +1,13 @@
parameters:
ignoreErrors:
-
message: '#^Call to method addHTML\(\) on an unknown class OutputPage\.$#'
identifier: class.notFound
message: '#^Method ProfessionalWiki\\WikibaseFacetedSearch\\EntryPoints\\WikibaseFacetedSearchHooks\:\:rewriteLinkForNonEntityResult\(\) never assigns null to &\$titleSnippet so it can be removed from the by\-ref type\.$#'
identifier: parameterByRef.unusedType
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Call to method addModuleStyles\(\) on an unknown class OutputPage\.$#'
identifier: class.notFound
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Call to method getNamespace\(\) on an unknown class Title\.$#'
identifier: class.notFound
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Call to static method element\(\) on an unknown class Html\.$#'
identifier: class.notFound
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Constant WB_NS_ITEM not found\.$#'
identifier: constant.notFound
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Parameter \$output of method ProfessionalWiki\\WikibaseFacetedSearch\\EntryPoints\\WikibaseFacetedSearchHooks\:\:onSpecialSearchResultsAppend\(\) has invalid type OutputPage\.$#'
identifier: class.notFound
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Parameter \$specialSearch of method ProfessionalWiki\\WikibaseFacetedSearch\\EntryPoints\\WikibaseFacetedSearchHooks\:\:onShowSearchHitTitle\(\) has invalid type SpecialSearch\.$#'
identifier: class.notFound
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Parameter \$specialSearch of method ProfessionalWiki\\WikibaseFacetedSearch\\EntryPoints\\WikibaseFacetedSearchHooks\:\:onSpecialSearchResultsAppend\(\) has invalid type SpecialSearch\.$#'
identifier: class.notFound
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php

-
message: '#^Parameter \$title of method ProfessionalWiki\\WikibaseFacetedSearch\\EntryPoints\\WikibaseFacetedSearchHooks\:\:onShowSearchHitTitle\(\) has invalid type Title\.$#'
identifier: class.notFound
message: '#^Method ProfessionalWiki\\WikibaseFacetedSearch\\EntryPoints\\WikibaseFacetedSearchHooks\:\:rewriteLinkForNonEntityResult\(\) never assigns string to &\$titleSnippet so it can be removed from the by\-ref type\.$#'
identifier: parameterByRef.unusedType
count: 1
path: src/EntryPoints/WikibaseFacetedSearchHooks.php
4 changes: 4 additions & 0 deletions phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,8 @@ parameters:
- ../../includes
- ../../tests/phpunit
- ../../vendor
- ../../extensions/CirrusSearch
- ../../extensions/Wikibase
- ../../extensions/WikibaseCirrusSearch
bootstrapFiles:
- ../../includes/AutoLoader.php
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for PHPStan to understand class_alias().

100 changes: 96 additions & 4 deletions src/EntryPoints/WikibaseFacetedSearchHooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,26 @@
namespace ProfessionalWiki\WikibaseFacetedSearch\EntryPoints;

use HtmlArmor;
use IContextSource;
use Language;
use OutputPage;
use ProfessionalWiki\WikibaseFacetedSearch\WikibaseFacetedSearchExtension;
use SearchResult;
use SpecialSearch;
use Title;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\Term\TermFallback;
use Wikibase\Lib\Store\LanguageFallbackLabelDescriptionLookup;
use Wikibase\Repo\Hooks\Formatters\EntityLinkFormatter;
use Wikibase\Repo\WikibaseRepo;
use Wikibase\Search\Elastic\EntityResult;

class WikibaseFacetedSearchHooks {

/**
* @param string[] $terms
* @param array<string, mixed> $query
* @param array<string, mixed> $attributes
* @param string[] $query
* @param string[] $attributes
*/
public static function onShowSearchHitTitle(
Title &$title,
Expand All @@ -26,11 +35,94 @@ public static function onShowSearchHitTitle(
array &$query,
array &$attributes
): void {
if ( $title->getNamespace() !== WB_NS_ITEM ) {
$itemId = self::getItemId( $title );

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

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

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

// TODO: get item site link and replace $title
$title = $pageTitle;
Copy link
Contributor Author

@malberts malberts Nov 30, 2024

Choose a reason for hiding this comment

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

Set the new target page.

For EntityResult results we just need to point to the new page, since the correct $titleSnippet gets set in CirrusShowSearchHitHandler::onShowSearchHitTitle().

For non-EntityResult we also need to rewrite the text below.


if ( !( $result instanceof EntityResult ) ) {
self::rewriteLinkForNonEntityResult(
self::newLabelDescriptionLookup( $specialSearch->getContext() ),
self::newLinkFormatter( $specialSearch->getLanguage() ),
$itemId,
$titleSnippet,
$attributes
);
}
}
Comment on lines +52 to +61
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For non-EntityResult there is no $titleSnippet by default, which means the link is going to use the text of the target page title, not the item title.

To get the item title again, this calls a minimal implementation based on HtmlPageLinkRendererEndHookHandler::internalDoHtmlPageLinkRendererEnd().


private static function getItemId( Title $title ): ?ItemId {
$entityId = WikibaseRepo::getEntityIdLookup()->getEntityIdForTitle( $title );

if ( $entityId instanceof ItemId ) {
return $entityId;
}

return null;
}

private static function getItemPage( ItemId $itemId ): ?Title {
return WikibaseFacetedSearchExtension::getInstance()->getItemPageLookup()->getPageTitle( $itemId );
}

private static function newLinkFormatter( Language $language ): EntityLinkFormatter {
return WikibaseRepo::getEntityLinkFormatterFactory()->getDefaultLinkFormatter( $language );
}

private static function newLabelDescriptionLookup( IContextSource $context ): LanguageFallbackLabelDescriptionLookup {
return new LanguageFallbackLabelDescriptionLookup(
WikibaseRepo::getTermLookup(),
WikibaseRepo::getLanguageFallbackChainFactory()->newFromContext( $context )
);
}

/**
* @param string[] $attributes
*/
private static function rewriteLinkForNonEntityResult(
LanguageFallbackLabelDescriptionLookup $labelDescriptionLookup,
EntityLinkFormatter $linkFormatter,
ItemId $itemId,
string|HtmlArmor|null &$titleSnippet,
array &$attributes
): void {
$labelData = self::termFallbackToTermData(
$labelDescriptionLookup->getLabel( $itemId )
);
$descriptionData = self::termFallbackToTermData(
$labelDescriptionLookup->getDescription( $itemId )
);

$titleSnippet = new HtmlArmor( $linkFormatter->getHtml( $itemId, $labelData ) );

$attributes['title'] = $linkFormatter->getTitleAttribute(
$itemId,
$labelData,
$descriptionData
);
}

/**
* @return string[]|null
*/
private static function termFallbackToTermData( ?TermFallback $term = null ): ?array {
if ( $term ) {
return [
'value' => $term->getText(),
'language' => $term->getActualLanguageCode(),
];
}

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.

There is a lot of static code here. Likely we can move much to one or more classes that do not bind to globals and can be tested easier

}

public static function onSpecialSearchResultsAppend(
Expand Down
14 changes: 14 additions & 0 deletions src/Persistence/ItemPageLookup.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

declare( strict_types = 1 );

namespace ProfessionalWiki\WikibaseFacetedSearch\Persistence;

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

interface ItemPageLookup {

public function getPageTitle( ItemId $itemId ): ?Title;

}
32 changes: 32 additions & 0 deletions src/Persistence/SiteLinkItemPageLookup.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

declare( strict_types = 1 );

namespace ProfessionalWiki\WikibaseFacetedSearch\Persistence;

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

class SiteLinkItemPageLookup implements ItemPageLookup {

public function __construct(
private readonly SiteLinkLookup $siteLinksStore,
private readonly string $siteLinkSiteId
) {
}

public function getPageTitle( ItemId $itemId ): ?Title {
$siteLink = array_filter(
$this->siteLinksStore->getSiteLinksForItem( $itemId ),
fn( $siteLink ) => $siteLink->getSiteId() === $this->siteLinkSiteId
)[0] ?? null;
malberts marked this conversation as resolved.
Show resolved Hide resolved

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

return Title::newFromText( $siteLink->getPageName() );
}

}
12 changes: 12 additions & 0 deletions src/WikibaseFacetedSearchExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

namespace ProfessionalWiki\WikibaseFacetedSearch;

use ProfessionalWiki\WikibaseFacetedSearch\Persistence\ItemPageLookup;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\SiteLinkItemPageLookup;
use Wikibase\Repo\WikibaseRepo;

class WikibaseFacetedSearchExtension {

public static function getInstance(): self {
Expand All @@ -13,4 +17,12 @@ public static function getInstance(): self {
return $instance;
}

public function getItemPageLookup(): ItemPageLookup {
return new SiteLinkItemPageLookup(
WikibaseRepo::getStore()->newSiteLinkStore(),
// TODO: https://github.com/ProfessionalWiki/WikibaseFacetedSearch/issues/12
'mardi'
);
}

}
70 changes: 70 additions & 0 deletions tests/Persistence/SiteLinkItemPageLookupTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?php

declare( strict_types = 1 );

namespace ProfessionalWiki\WikibaseFacetedSearch\Tests\Persistence;

use PHPUnit\Framework\TestCase;
use ProfessionalWiki\WikibaseFacetedSearch\Persistence\SiteLinkItemPageLookup;
use Title;
use Wikibase\DataModel\Entity\Item;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\Lib\Store\HashSiteLinkStore;

/**
* @covers \ProfessionalWiki\WikibaseFacetedSearch\Persistence\SiteLinkItemPageLookup
*/
class SiteLinkItemPageLookupTest extends TestCase {

private const SITE_ID = 'testSiteId';

private HashSiteLinkStore $siteLinkStore;
private SiteLinkItemPageLookup $lookup;

protected function setUp(): void {
$this->siteLinkStore = new HashSiteLinkStore();
$this->lookup = new SiteLinkItemPageLookup(
$this->siteLinkStore,
self::SITE_ID
);
}

public function testReturnsPageWhenSiteLinkExists(): void {
$this->createItemWithSiteLink( 'Q42', self::SITE_ID, 'Page for Q42' );

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

private function createItemWithSiteLink( string $itemId, string $siteId, string $pageName ): void {
$item = new Item( new ItemId( $itemId ) );
$item->getSiteLinkList()->addNewSiteLink( $siteId, $pageName );
$this->siteLinkStore->saveLinksOfItem( $item );
}

private function assertItemPageHasTitle( string $itemId, $pageTitle ): void {
$this->assertEquals(
Title::newFromText( $pageTitle ),
$this->lookup->getPageTitle( new ItemId( $itemId ) )
);
}

public function testReturnsNullWhenNoSiteLinksExist(): void {
$this->assertNull( $this->lookup->getPageTitle( new ItemId( 'Q404' ) ) );
}

public function testReturnsNullWhenOnlyOtherSiteLinksExist(): void {
$this->createItemWithSiteLink( 'Q100', 'otherSiteId', 'Other page' );
$this->createItemWithSiteLink( 'Q200', 'anotherSiteId', 'Another page' );

$this->assertNull( $this->lookup->getPageTitle( new ItemId( 'Q42' ) ) );
}

public function testReturnsPageWhenManySiteLinksExist(): void {
$this->createItemWithSiteLink( 'Q42', self::SITE_ID, 'Page for Q42' );
$this->createItemWithSiteLink( 'Q100', 'otherSiteId', 'Other page' );
$this->createItemWithSiteLink( 'Q200', 'anotherSiteId', 'Another page' );
malberts marked this conversation as resolved.
Show resolved Hide resolved

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

}