From 650570e50a847c31fc9abea83e3025e2884270b3 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Mon, 5 May 2025 13:49:39 -0400 Subject: [PATCH 01/10] Add Resolver page --- PersistentPageIdentifiers.alias.php | 7 +++ extension.json | 5 ++ i18n/en.json | 5 +- i18n/qqq.json | 5 +- src/Adapters/PageIdsRepo.php | 11 ++++ src/EntryPoints/SpecialPURIResolver.php | 77 +++++++++++++++++++++++++ tests/Adapters/PageIdsRepoTest.php | 17 ++++++ 7 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 PersistentPageIdentifiers.alias.php create mode 100644 src/EntryPoints/SpecialPURIResolver.php diff --git a/PersistentPageIdentifiers.alias.php b/PersistentPageIdentifiers.alias.php new file mode 100644 index 0000000..494c0db --- /dev/null +++ b/PersistentPageIdentifiers.alias.php @@ -0,0 +1,7 @@ + [ 'PURIResolver' ], +]; diff --git a/extension.json b/extension.json index 447623f..0d2e382 100644 --- a/extension.json +++ b/extension.json @@ -61,7 +61,12 @@ } ], + "SpecialPages": { + "PURIResolver": "ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPURIResolver" + }, + "ExtensionMessagesFiles": { + "PersistentPageIdentifiersAlias": "PersistentPageIdentifiers.alias.php", "PersistentPageIdentifiersMagic": "i18n/Magic/MagicWords.php" }, diff --git a/i18n/en.json b/i18n/en.json index 8d59222..d79595e 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -8,5 +8,8 @@ }, "persistentpageidentifiers-name": "Persistent Page Identifiers", "persistentpageidentifiers-description": "Stable unique identifiers for your wiki pages. UUID v7 or PURIs, accessible via parser function and REST API", - "persistentpageidentifiers-info-label": "Persistent page identifier" + "persistentpageidentifiers-info-label": "Persistent page identifier", + "puriresolver": "Redirect by PURI", + "puriresolver-puri": "PURI", + "puriresolver-not-exists": "PURI does not exist" } diff --git a/i18n/qqq.json b/i18n/qqq.json index 3882b01..16b318c 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -7,5 +7,8 @@ }, "persistentpageidentifiers-name": "{{Name}}", "persistentpageidentifiers-description": "{{Desc|name=PersistentPageIdentifiers|url=https://github.com/ProfessionalWiki/PersistentPageIdentifiers}}", - "persistentpageidentifiers-info-label": "Persistent page identifier label on action=info page" + "persistentpageidentifiers-info-label": "Persistent page identifier label on action=info page", + "puriresolver": "Special page name for redirect by PURI", + "puriresolver-puri": "Label for PURI input field", + "puriresolver-not-exists": "Error message when PURI does not exist" } diff --git a/src/Adapters/PageIdsRepo.php b/src/Adapters/PageIdsRepo.php index 868c9da..206a546 100644 --- a/src/Adapters/PageIdsRepo.php +++ b/src/Adapters/PageIdsRepo.php @@ -30,4 +30,15 @@ public function getPageIdsOfPagesWithoutPersistentIds( int $limit ): array { ); } + public function getPageIdFromPersistentId( string $persistentId ): ?int { + $row = $this->database->newSelectQueryBuilder() + ->select( [ 'p.page_id', 'ppi.persistent_id' ] ) + ->from( 'page', 'p' ) + ->leftJoin( 'persistent_page_ids', 'ppi', 'p.page_id = ppi.page_id' ) + ->where( [ 'ppi.persistent_id' => $persistentId ] ) + ->fetchRow(); + + return $row ? (int)$row->page_id : null; + } + } diff --git a/src/EntryPoints/SpecialPURIResolver.php b/src/EntryPoints/SpecialPURIResolver.php new file mode 100644 index 0000000..351bbf3 --- /dev/null +++ b/src/EntryPoints/SpecialPURIResolver.php @@ -0,0 +1,77 @@ +getTitleFromPersistentId( $subPage ); + + if ( !$title ) { + return; + } + + $this->getOutput()->redirect( $title->getFullURL() ); + } + + protected function getFormFields() { + return [ + 'puri' => [ + 'type' => 'text', + 'label-message' => 'puriresolver-puri', + 'required' => true, + ] + ]; + } + + public function onSubmit( array $data ) { + $title = $this->getTitleFromPersistentId( $data['puri'] ); + + if ( !$title ) { + // Message: puri-not-exists + return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); + } + + $this->getOutput()->redirect( $title->getFullURL() ); + } + + protected function getDisplayFormat() { + return 'ooui'; + } + + public function getGroupName(): string { + return 'redirects'; + } + + private function getTitleFromPersistentId( string $persistentId ): ?Title { + $pageId = $this->getPageIdFromPersistentId( $persistentId ); + + if ( $pageId ) { + return Title::newFromID( $pageId ); + } + + return null; + } + + private function getPageIdFromPersistentId( string $persistentId ): ?int { + return PersistentPageIdentifiersExtension::getInstance()->getPageIdsRepo()->getPageIdFromPersistentId( $persistentId ); + } + +} diff --git a/tests/Adapters/PageIdsRepoTest.php b/tests/Adapters/PageIdsRepoTest.php index adc3f9b..6c93996 100644 --- a/tests/Adapters/PageIdsRepoTest.php +++ b/tests/Adapters/PageIdsRepoTest.php @@ -71,4 +71,21 @@ public function testReturnsPageIdsForPagesWithoutPersistentIdsUpToLimit(): void ); } + public function testGetPageIdFromPersistentId(): void { + $page = $this->createPageWithText(); + + $persistentId = $this->db->newSelectQueryBuilder() + ->select( 'ppi.persistent_id' ) + ->from( 'persistent_page_ids', 'ppi' ) + ->where( [ 'ppi.page_id' => $page->getId() ] ) + ->fetchField(); + + $this->assertIsString( $persistentId ); + $this->assertSame( $page->getId(), $this->repo->getPageIdFromPersistentId( $persistentId ) ); + } + + public function testGetPageIdFromNonExistentPersistentId(): void { + $this->assertNull( $this->repo->getPageIdFromPersistentId( 'non-existent' ) ); + } + } From 26f4f496301075c56e6d80f29132ab1a5538aaea Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Mon, 5 May 2025 14:15:12 -0400 Subject: [PATCH 02/10] Code review --- phpstan-baseline.neon | 12 ++++++++++++ src/Adapters/PageIdsRepo.php | 6 +++--- src/EntryPoints/SpecialPURIResolver.php | 8 +++++--- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index c3af1b1..3bdbbc5 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -30,6 +30,18 @@ parameters: count: 1 path: src/EntryPoints/PersistentPageIdentifiersHooks.php + - + message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPURIResolver\:\:getFormFields\(\) return type has no value type specified in iterable type array\.$#' + identifier: missingType.iterableValue + count: 1 + path: src/EntryPoints/SpecialPURIResolver.php + + - + message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPURIResolver\:\:onSubmit\(\) has parameter \$data with no value type specified in iterable type array\.$#' + identifier: missingType.iterableValue + count: 1 + path: src/EntryPoints/SpecialPURIResolver.php + - message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\PersistentPageIdentifiersExtension\:\:getDatabase\(\) should return Wikimedia\\Rdbms\\IDatabase but returns Wikimedia\\Rdbms\\IDatabase\|false\.$#' identifier: return.type diff --git a/src/Adapters/PageIdsRepo.php b/src/Adapters/PageIdsRepo.php index 206a546..99835a3 100644 --- a/src/Adapters/PageIdsRepo.php +++ b/src/Adapters/PageIdsRepo.php @@ -32,13 +32,13 @@ public function getPageIdsOfPagesWithoutPersistentIds( int $limit ): array { public function getPageIdFromPersistentId( string $persistentId ): ?int { $row = $this->database->newSelectQueryBuilder() - ->select( [ 'p.page_id', 'ppi.persistent_id' ] ) + ->select( 'p.page_id' ) ->from( 'page', 'p' ) - ->leftJoin( 'persistent_page_ids', 'ppi', 'p.page_id = ppi.page_id' ) + ->join( 'persistent_page_ids', 'ppi', 'p.page_id = ppi.page_id' ) ->where( [ 'ppi.persistent_id' => $persistentId ] ) ->fetchRow(); - return $row ? (int)$row->page_id : null; + return $row && is_object( $row ) ? (int)$row->page_id : null; } } diff --git a/src/EntryPoints/SpecialPURIResolver.php b/src/EntryPoints/SpecialPURIResolver.php index 351bbf3..e3b1c3d 100644 --- a/src/EntryPoints/SpecialPURIResolver.php +++ b/src/EntryPoints/SpecialPURIResolver.php @@ -31,7 +31,7 @@ public function execute( $subPage ): void { $this->getOutput()->redirect( $title->getFullURL() ); } - protected function getFormFields() { + protected function getFormFields(): array { return [ 'puri' => [ 'type' => 'text', @@ -41,7 +41,7 @@ protected function getFormFields() { ]; } - public function onSubmit( array $data ) { + public function onSubmit( array $data ): Status|bool { $title = $this->getTitleFromPersistentId( $data['puri'] ); if ( !$title ) { @@ -50,9 +50,11 @@ public function onSubmit( array $data ) { } $this->getOutput()->redirect( $title->getFullURL() ); + + return true; } - protected function getDisplayFormat() { + protected function getDisplayFormat(): string { return 'ooui'; } From d0d79a658420c84ac064f1c53a09230b55b4078d Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 16:39:09 -0400 Subject: [PATCH 03/10] Code review --- PersistentPageIdentifiers.alias.php | 2 +- extension.json | 2 +- i18n/en.json | 5 ++--- i18n/qqq.json | 5 ++--- phpstan-baseline.neon | 8 +++---- .../DatabasePersistentPageIdentifiersRepo.php | 10 +++++++++ src/Adapters/PageIdsRepo.php | 11 ---------- .../StubPersistentPageIdentifiersRepo.php | 4 ++++ .../PersistentPageIdentifiersRepo.php | 1 + ...ecialPersistentPageIdentifierResolver.php} | 22 +++++++++---------- ...abasePersistentPageIdentifiersRepoTest.php | 12 ++++++++++ tests/Adapters/PageIdsRepoTest.php | 17 -------------- .../InMemoryPersistentPageIdentifiersRepo.php | 5 +++++ 13 files changed, 53 insertions(+), 51 deletions(-) rename src/EntryPoints/{SpecialPURIResolver.php => SpecialPersistentPageIdentifierResolver.php} (69%) diff --git a/PersistentPageIdentifiers.alias.php b/PersistentPageIdentifiers.alias.php index 494c0db..2aa203c 100644 --- a/PersistentPageIdentifiers.alias.php +++ b/PersistentPageIdentifiers.alias.php @@ -3,5 +3,5 @@ /** English (English) */ $specialPageAliases['en'] = [ - 'PURIResolver' => [ 'PURIResolver' ], + 'PersistentPageIdentifierResolver' => [ 'PersistentPageIdentifierResolver' ], ]; diff --git a/extension.json b/extension.json index 0d2e382..0f66eef 100644 --- a/extension.json +++ b/extension.json @@ -62,7 +62,7 @@ ], "SpecialPages": { - "PURIResolver": "ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPURIResolver" + "PersistentPageIdentifierResolver": "ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPersistentPageIdentifierResolver" }, "ExtensionMessagesFiles": { diff --git a/i18n/en.json b/i18n/en.json index d79595e..9f58662 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -9,7 +9,6 @@ "persistentpageidentifiers-name": "Persistent Page Identifiers", "persistentpageidentifiers-description": "Stable unique identifiers for your wiki pages. UUID v7 or PURIs, accessible via parser function and REST API", "persistentpageidentifiers-info-label": "Persistent page identifier", - "puriresolver": "Redirect by PURI", - "puriresolver-puri": "PURI", - "puriresolver-not-exists": "PURI does not exist" + "persistentpageidentifierresolver": "Redirect by Persistent Page Identifier", + "persistentpageidentifierresolver-not-exists": "Persistent Page Identifier does not exist" } diff --git a/i18n/qqq.json b/i18n/qqq.json index 16b318c..135083f 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -8,7 +8,6 @@ "persistentpageidentifiers-name": "{{Name}}", "persistentpageidentifiers-description": "{{Desc|name=PersistentPageIdentifiers|url=https://github.com/ProfessionalWiki/PersistentPageIdentifiers}}", "persistentpageidentifiers-info-label": "Persistent page identifier label on action=info page", - "puriresolver": "Special page name for redirect by PURI", - "puriresolver-puri": "Label for PURI input field", - "puriresolver-not-exists": "Error message when PURI does not exist" + "persistentpageidentifierresolver": "Special page name for redirect by Persistent Page Identifier", + "persistentpageidentifierresolver-not-exists": "Error message when Persistent Page Identifier does not exist" } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 3bdbbc5..7c51f05 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -31,16 +31,16 @@ parameters: path: src/EntryPoints/PersistentPageIdentifiersHooks.php - - message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPURIResolver\:\:getFormFields\(\) return type has no value type specified in iterable type array\.$#' + message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPersistentPageIdentifierResolver\:\:getFormFields\(\) return type has no value type specified in iterable type array\.$#' identifier: missingType.iterableValue count: 1 - path: src/EntryPoints/SpecialPURIResolver.php + path: src/EntryPoints/SpecialPersistentPageIdentifierResolver.php - - message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPURIResolver\:\:onSubmit\(\) has parameter \$data with no value type specified in iterable type array\.$#' + message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\EntryPoints\\SpecialPersistentPageIdentifierResolver\:\:onSubmit\(\) has parameter \$data with no value type specified in iterable type array\.$#' identifier: missingType.iterableValue count: 1 - path: src/EntryPoints/SpecialPURIResolver.php + path: src/EntryPoints/SpecialPersistentPageIdentifierResolver.php - message: '#^Method ProfessionalWiki\\PersistentPageIdentifiers\\PersistentPageIdentifiersExtension\:\:getDatabase\(\) should return Wikimedia\\Rdbms\\IDatabase but returns Wikimedia\\Rdbms\\IDatabase\|false\.$#' diff --git a/src/Adapters/DatabasePersistentPageIdentifiersRepo.php b/src/Adapters/DatabasePersistentPageIdentifiersRepo.php index d34d932..207b916 100644 --- a/src/Adapters/DatabasePersistentPageIdentifiersRepo.php +++ b/src/Adapters/DatabasePersistentPageIdentifiersRepo.php @@ -59,4 +59,14 @@ private function persistentIdsResultToArray( IResultWrapper $result ): array { return $rows; } + public function getPageIdFromPersistentId( string $persistentId ): ?int { + $pageId = $this->database->newSelectQueryBuilder() + ->select( 'page_id' ) + ->from( 'persistent_page_ids' ) + ->where( [ 'persistent_id' => $persistentId ] ) + ->fetchField(); + + return is_numeric( $pageId ) ? (int)$pageId : null; + } + } diff --git a/src/Adapters/PageIdsRepo.php b/src/Adapters/PageIdsRepo.php index 99835a3..868c9da 100644 --- a/src/Adapters/PageIdsRepo.php +++ b/src/Adapters/PageIdsRepo.php @@ -30,15 +30,4 @@ public function getPageIdsOfPagesWithoutPersistentIds( int $limit ): array { ); } - public function getPageIdFromPersistentId( string $persistentId ): ?int { - $row = $this->database->newSelectQueryBuilder() - ->select( 'p.page_id' ) - ->from( 'page', 'p' ) - ->join( 'persistent_page_ids', 'ppi', 'p.page_id = ppi.page_id' ) - ->where( [ 'ppi.persistent_id' => $persistentId ] ) - ->fetchRow(); - - return $row && is_object( $row ) ? (int)$row->page_id : null; - } - } diff --git a/src/Adapters/StubPersistentPageIdentifiersRepo.php b/src/Adapters/StubPersistentPageIdentifiersRepo.php index 70deae3..2533e5d 100644 --- a/src/Adapters/StubPersistentPageIdentifiersRepo.php +++ b/src/Adapters/StubPersistentPageIdentifiersRepo.php @@ -25,4 +25,8 @@ public function getPersistentIds( array $pageIds ): array { return array_combine( $pageIds, array_fill( 0, count( $pageIds ), $this->id ) ); } + public function getPageIdFromPersistentId( string $persistentId ): ?int { + return $this->id ? 1337 : null; + } + } diff --git a/src/Application/PersistentPageIdentifiersRepo.php b/src/Application/PersistentPageIdentifiersRepo.php index 2ffcae2..688623e 100644 --- a/src/Application/PersistentPageIdentifiersRepo.php +++ b/src/Application/PersistentPageIdentifiersRepo.php @@ -19,4 +19,5 @@ public function getPersistentId( int $pageId ): ?string; */ public function getPersistentIds( array $pageIds ): array; + public function getPageIdFromPersistentId( string $persistentId ): ?int; } diff --git a/src/EntryPoints/SpecialPURIResolver.php b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php similarity index 69% rename from src/EntryPoints/SpecialPURIResolver.php rename to src/EntryPoints/SpecialPersistentPageIdentifierResolver.php index e3b1c3d..687e818 100644 --- a/src/EntryPoints/SpecialPURIResolver.php +++ b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php @@ -9,22 +9,22 @@ use Status; use Title; -class SpecialPURIResolver extends FormSpecialPage { +class SpecialPersistentPageIdentifierResolver extends FormSpecialPage { public function __construct() { - parent::__construct( 'PURIResolver' ); + parent::__construct( 'PersistentPageIdentifierResolver' ); } public function execute( $subPage ): void { parent::execute( $subPage ); - if ( !$subPage ) { + if ( $subPage === null || $subPage === '' ) { return; } $title = $this->getTitleFromPersistentId( $subPage ); - if ( !$title ) { + if ( $title === null ) { return; } @@ -33,19 +33,19 @@ public function execute( $subPage ): void { protected function getFormFields(): array { return [ - 'puri' => [ + 'persistentpageidentifier' => [ 'type' => 'text', - 'label-message' => 'puriresolver-puri', + 'label-message' => 'persistentpageidentifiers-info-label', 'required' => true, ] ]; } public function onSubmit( array $data ): Status|bool { - $title = $this->getTitleFromPersistentId( $data['puri'] ); + $title = $this->getTitleFromPersistentId( $data['persistentpageidentifier'] ); - if ( !$title ) { - // Message: puri-not-exists + if ( $title === null ) { + // Message: persistentpageidentifierresolver-not-exists return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } @@ -65,7 +65,7 @@ public function getGroupName(): string { private function getTitleFromPersistentId( string $persistentId ): ?Title { $pageId = $this->getPageIdFromPersistentId( $persistentId ); - if ( $pageId ) { + if ( $pageId !== null ) { return Title::newFromID( $pageId ); } @@ -73,7 +73,7 @@ private function getTitleFromPersistentId( string $persistentId ): ?Title { } private function getPageIdFromPersistentId( string $persistentId ): ?int { - return PersistentPageIdentifiersExtension::getInstance()->getPageIdsRepo()->getPageIdFromPersistentId( $persistentId ); + return PersistentPageIdentifiersExtension::getInstance()->getPersistentPageIdentifiersRepo()->getPageIdFromPersistentId( $persistentId ); } } diff --git a/tests/Adapters/DatabasePersistentPageIdentifiersRepoTest.php b/tests/Adapters/DatabasePersistentPageIdentifiersRepoTest.php index 6bcd3d7..6d71aee 100644 --- a/tests/Adapters/DatabasePersistentPageIdentifiersRepoTest.php +++ b/tests/Adapters/DatabasePersistentPageIdentifiersRepoTest.php @@ -102,4 +102,16 @@ public function testCanSaveAndRetrieveMultiplePersistentIds(): void { ); } + public function testGetPageIdFromPersistentId(): void { + $pageId = $this->createPageWithText()->getId(); + $persistentId = '00000000-0000-0000-0000-000000000042'; + + $this->repo->savePersistentIds( [ $pageId => $persistentId ] ); + $this->assertSame( $pageId, $this->repo->getPageIdFromPersistentId( $persistentId ) ); + } + + public function testGetPageIdFromNonExistentPersistentId(): void { + $this->assertNull( $this->repo->getPageIdFromPersistentId( 'non-existent' ) ); + } + } diff --git a/tests/Adapters/PageIdsRepoTest.php b/tests/Adapters/PageIdsRepoTest.php index 6c93996..adc3f9b 100644 --- a/tests/Adapters/PageIdsRepoTest.php +++ b/tests/Adapters/PageIdsRepoTest.php @@ -71,21 +71,4 @@ public function testReturnsPageIdsForPagesWithoutPersistentIdsUpToLimit(): void ); } - public function testGetPageIdFromPersistentId(): void { - $page = $this->createPageWithText(); - - $persistentId = $this->db->newSelectQueryBuilder() - ->select( 'ppi.persistent_id' ) - ->from( 'persistent_page_ids', 'ppi' ) - ->where( [ 'ppi.page_id' => $page->getId() ] ) - ->fetchField(); - - $this->assertIsString( $persistentId ); - $this->assertSame( $page->getId(), $this->repo->getPageIdFromPersistentId( $persistentId ) ); - } - - public function testGetPageIdFromNonExistentPersistentId(): void { - $this->assertNull( $this->repo->getPageIdFromPersistentId( 'non-existent' ) ); - } - } diff --git a/tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php b/tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php index 2d8331b..08062b5 100644 --- a/tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php +++ b/tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php @@ -22,4 +22,9 @@ public function getPersistentIds( array $pageIds ): array { return array_intersect_key( $this->persistentIds, array_flip( $pageIds ) ); } + public function getPageIdFromPersistentId( string $persistentId ): ?int { + $pageId = array_search( $persistentId, $this->persistentIds, true ); + return $pageId === false ? null : $pageId; + } + } From 61481e2696d4d6c7811eace5152a0ad4b1a374ed Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 18:09:16 -0400 Subject: [PATCH 04/10] Add integration test for SpecialPersistentPageIdentifierResolver --- ...pecialPersistentPageIdentifierResolver.php | 2 - ...tPageIdentifierResolverIntegrationTest.php | 117 ++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php diff --git a/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php index 687e818..f556d5c 100644 --- a/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php +++ b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php @@ -16,8 +16,6 @@ public function __construct() { } public function execute( $subPage ): void { - parent::execute( $subPage ); - if ( $subPage === null || $subPage === '' ) { return; } diff --git a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php new file mode 100644 index 0000000..269c903 --- /dev/null +++ b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php @@ -0,0 +1,117 @@ +tablesUsed[] = 'persistent_page_ids'; + $this->repo = new DatabasePersistentPageIdentifiersRepo( $this->db ); + } + + protected function newSpecialPage(): SpecialPersistentPageIdentifierResolver { + return new SpecialPersistentPageIdentifierResolver(); + } + + public function testExecuteWithValidIdRedirects(): void { + $page = $this->createPageWithText(); + + $resolver = $this->newSpecialPage(); + $resolver->execute( $this->repo->getPersistentId( $page->getId() ) ); + + $this->assertSame( + $page->getTitle()->getFullURL(), + $resolver->getOutput()->getRedirect(), + 'Should redirect to the page associated with the persistent ID.' + ); + } + + public function testExecuteWithNonExistentIdReturns(): void { + $resolver = $this->newSpecialPage(); + $resolver->execute( 'non-existent-id' ); + + $this->assertSame( + '', + $resolver->getOutput()->getRedirect(), + 'Should not redirect for a non-existent persistent ID.' + ); + } + + public function testExecuteWithEmptySubPageReturns(): void { + $resolver = $this->newSpecialPage(); + $resolver->execute( '' ); + + $this->assertSame( + '', + $resolver->getOutput()->getRedirect(), + 'Should not redirect for an empty subpage.' + ); + } + + public function testExecuteWithNullSubPageReturns(): void { + $resolver = $this->newSpecialPage(); + $resolver->execute( null ); + + $this->assertSame( + '', + $resolver->getOutput()->getRedirect(), + 'Should not redirect for a null subpage.' + ); + } + + public function testOnSubmitWithValidIdRedirects(): void { + $page = $this->createPageWithText(); + + $resolver = $this->newSpecialPage(); + $status = $resolver->onSubmit( [ 'persistentpageidentifier' => $this->repo->getPersistentId( $page->getId() ) ] ); + + $this->assertTrue( $status, 'onSubmit should return true on success.' ); + $this->assertSame( + $page->getTitle()->getFullURL(), + $resolver->getOutput()->getRedirect(), + 'Should redirect to the page associated with the persistent ID on form submission.' + ); + } + + public function testOnSubmitWithNonExistentIdReturnsFatalStatus(): void { + $resolver = $this->newSpecialPage(); + $status = $resolver->onSubmit( [ 'persistentpageidentifier' => 'non-existent-id' ] ); + + $this->assertInstanceOf( + Status::class, + $status, + 'onSubmit should return a Status object for a non-existent ID.' + ); + $this->assertFalse( $status->isGood(), 'Status should be fatal.' ); + + $errors = $status->getErrors(); + $this->assertNotEmpty( $errors, 'Status should contain errors.' ); + $this->assertEquals( + 'persistentpageidentifierresolver-not-exists', + $errors[0]['message'], + 'Status message should indicate the ID does not exist.' + ); + + $this->assertSame( + '', + $resolver->getOutput()->getRedirect(), + 'Should not redirect when ID does not exist on form submission.' + ); + } + +} From dfc6776a98dc69ae66914608b96184e33176b3f3 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 18:49:47 -0400 Subject: [PATCH 05/10] Redirect immediately if the subpage is valid --- .../SpecialPersistentPageIdentifierResolver.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php index f556d5c..151d36b 100644 --- a/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php +++ b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php @@ -16,17 +16,27 @@ public function __construct() { } public function execute( $subPage ): void { - if ( $subPage === null || $subPage === '' ) { + // Redirect to the page immediately if it is valid + if ( $this->getRedirectUrl( $subPage ) !== null ) { + $this->getOutput()->redirect( $this->getRedirectUrl( $subPage ) ); return; } + parent::execute( $subPage ); + } + + private function getRedirectUrl( ?string $subPage ): ?string { + if ( $subPage === null || $subPage === '' ) { + return null; + } + $title = $this->getTitleFromPersistentId( $subPage ); if ( $title === null ) { - return; + return null; } - $this->getOutput()->redirect( $title->getFullURL() ); + return $title->getFullURL(); } protected function getFormFields(): array { From 892d3e504cd3e355c2c2da912a058cb3bfdac00c Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 20:01:47 -0400 Subject: [PATCH 06/10] Match both ID and custom format --- ...pecialPersistentPageIdentifierResolver.php | 47 ++++++++++--------- .../PersistentPageIdFormatter.php | 6 +++ .../PersistentPageIdFormatterTest.php | 9 ++++ 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php index 151d36b..5c6b076 100644 --- a/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php +++ b/src/EntryPoints/SpecialPersistentPageIdentifierResolver.php @@ -6,6 +6,7 @@ use FormSpecialPage; use ProfessionalWiki\PersistentPageIdentifiers\PersistentPageIdentifiersExtension; +use ProfessionalWiki\PersistentPageIdentifiers\Presentation\PersistentPageIdFormatter; use Status; use Title; @@ -17,28 +18,15 @@ public function __construct() { public function execute( $subPage ): void { // Redirect to the page immediately if it is valid - if ( $this->getRedirectUrl( $subPage ) !== null ) { - $this->getOutput()->redirect( $this->getRedirectUrl( $subPage ) ); + $url = $this->getUrlFromPersistentId( $subPage ); + if ( $url !== null ) { + $this->getOutput()->redirect( $url ); return; } parent::execute( $subPage ); } - private function getRedirectUrl( ?string $subPage ): ?string { - if ( $subPage === null || $subPage === '' ) { - return null; - } - - $title = $this->getTitleFromPersistentId( $subPage ); - - if ( $title === null ) { - return null; - } - - return $title->getFullURL(); - } - protected function getFormFields(): array { return [ 'persistentpageidentifier' => [ @@ -50,15 +38,12 @@ protected function getFormFields(): array { } public function onSubmit( array $data ): Status|bool { - $title = $this->getTitleFromPersistentId( $data['persistentpageidentifier'] ); - - if ( $title === null ) { - // Message: persistentpageidentifierresolver-not-exists + $url = $this->getUrlFromPersistentId( $data['persistentpageidentifier'] ); + if ( $url === null ) { return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } - $this->getOutput()->redirect( $title->getFullURL() ); - + $this->getOutput()->redirect( $url ); return true; } @@ -70,6 +55,20 @@ public function getGroupName(): string { return 'redirects'; } + private function getUrlFromPersistentId( ?string $persistentId ): ?string { + if ( $persistentId === null || $persistentId === '' ) { + return null; + } + + $title = $this->getTitleFromPersistentId( $this->extractId( $persistentId ) ); + + if ( $title === null || !$title->exists() ) { + return null; + } + + return $title->getFullURL(); + } + private function getTitleFromPersistentId( string $persistentId ): ?Title { $pageId = $this->getPageIdFromPersistentId( $persistentId ); @@ -84,4 +83,8 @@ private function getPageIdFromPersistentId( string $persistentId ): ?int { return PersistentPageIdentifiersExtension::getInstance()->getPersistentPageIdentifiersRepo()->getPageIdFromPersistentId( $persistentId ); } + private function extractId( string $input ): string { + return PersistentPageIdentifiersExtension::getInstance()->newPersistentPageIdFormatter()->extractId( $input ); + } + } diff --git a/src/Presentation/PersistentPageIdFormatter.php b/src/Presentation/PersistentPageIdFormatter.php index 4846b02..6e64d9f 100644 --- a/src/Presentation/PersistentPageIdFormatter.php +++ b/src/Presentation/PersistentPageIdFormatter.php @@ -19,4 +19,10 @@ public function format( ?string $persistentId ): string { return str_replace( '$1', $persistentId, $this->format ); } + public function extractId( string $input ): string { + // \$1 because it is escaped in the format string + $pattern = '/^' . str_replace( '\$1', '(.*?)', preg_quote( $this->format, '/' ) ) . '$/'; + return preg_match( $pattern, $input, $matches ) ? $matches[1] : $input; + } + } diff --git a/tests/Presentation/PersistentPageIdFormatterTest.php b/tests/Presentation/PersistentPageIdFormatterTest.php index 03068d9..e98bf5f 100644 --- a/tests/Presentation/PersistentPageIdFormatterTest.php +++ b/tests/Presentation/PersistentPageIdFormatterTest.php @@ -22,4 +22,13 @@ public function testReturnsFormattedId(): void { $this->assertSame( 'foo 42 bar', $formatter->format( '42' ) ); } + public function testExtractsIdMatchingFormat(): void { + $formatter = new PersistentPageIdFormatter( 'foo $1 bar' ); + $this->assertSame( '42', $formatter->extractId( 'foo 42 bar' ) ); + } + + public function testExtractsIdNotMatchingFormat(): void { + $formatter = new PersistentPageIdFormatter( 'foo $1 bar' ); + $this->assertSame( 'bar 42 foo', $formatter->extractId( 'bar 42 foo' ) ); + } } From 74d021054bfc798e938d02842acab89476806138 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 20:01:55 -0400 Subject: [PATCH 07/10] Clean up pages after test --- ...tPageIdentifierResolverIntegrationTest.php | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php index 269c903..653bc45 100644 --- a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php +++ b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php @@ -9,6 +9,7 @@ use ProfessionalWiki\PersistentPageIdentifiers\EntryPoints\SpecialPersistentPageIdentifierResolver; use ProfessionalWiki\PersistentPageIdentifiers\Tests\PersistentPageIdentifiersIntegrationTest; use Status; +use WikiPage; /** * @covers \ProfessionalWiki\PersistentPageIdentifiers\EntryPoints\SpecialPersistentPageIdentifierResolver @@ -17,6 +18,7 @@ class SpecialPersistentPageIdentifierResolverIntegrationTest extends PersistentPageIdentifiersIntegrationTest { private PersistentPageIdentifiersRepo $repo; + private WikiPage $page; protected function setUp(): void { parent::setUp(); @@ -24,18 +26,27 @@ protected function setUp(): void { $this->repo = new DatabasePersistentPageIdentifiersRepo( $this->db ); } + protected function tearDown(): void { + // This is required because the pages persist into GenerateMissingIdentifiersTest in + // older MediaWiki/PHPUnit version. This is not needed for MW 1.42+. + if ( isset( $this->page ) ) { + $this->deletePage( $this->page ); + } + parent::tearDown(); + } + protected function newSpecialPage(): SpecialPersistentPageIdentifierResolver { return new SpecialPersistentPageIdentifierResolver(); } public function testExecuteWithValidIdRedirects(): void { - $page = $this->createPageWithText(); + $this->page = $this->createPageWithText(); $resolver = $this->newSpecialPage(); - $resolver->execute( $this->repo->getPersistentId( $page->getId() ) ); + $resolver->execute( $this->repo->getPersistentId( $this->page->getId() ) ); $this->assertSame( - $page->getTitle()->getFullURL(), + $this->page->getTitle()->getFullURL(), $resolver->getOutput()->getRedirect(), 'Should redirect to the page associated with the persistent ID.' ); @@ -43,6 +54,9 @@ public function testExecuteWithValidIdRedirects(): void { public function testExecuteWithNonExistentIdReturns(): void { $resolver = $this->newSpecialPage(); + // Call parent::execute first before attempting to execute with an invalid subpage + // This is only needed for the test + // $resolver->getContext()->setTitle( $resolver->getPageTitle() ); $resolver->execute( 'non-existent-id' ); $this->assertSame( @@ -54,6 +68,9 @@ public function testExecuteWithNonExistentIdReturns(): void { public function testExecuteWithEmptySubPageReturns(): void { $resolver = $this->newSpecialPage(); + // Call parent::execute first before attempting to execute with an empty subpage + // This is only needed for the test + // $resolver->getContext()->setTitle( $resolver->getPageTitle() ); $resolver->execute( '' ); $this->assertSame( @@ -65,6 +82,9 @@ public function testExecuteWithEmptySubPageReturns(): void { public function testExecuteWithNullSubPageReturns(): void { $resolver = $this->newSpecialPage(); + // Call parent::execute first before attempting to execute with a null subpage + // This is only needed for the test + // $resolver->getContext()->setTitle( $resolver->getPageTitle() ); $resolver->execute( null ); $this->assertSame( @@ -75,14 +95,14 @@ public function testExecuteWithNullSubPageReturns(): void { } public function testOnSubmitWithValidIdRedirects(): void { - $page = $this->createPageWithText(); + $this->page = $this->createPageWithText(); $resolver = $this->newSpecialPage(); - $status = $resolver->onSubmit( [ 'persistentpageidentifier' => $this->repo->getPersistentId( $page->getId() ) ] ); + $status = $resolver->onSubmit( [ 'persistentpageidentifier' => $this->repo->getPersistentId( $this->page->getId() ) ] ); $this->assertTrue( $status, 'onSubmit should return true on success.' ); $this->assertSame( - $page->getTitle()->getFullURL(), + $this->page->getTitle()->getFullURL(), $resolver->getOutput()->getRedirect(), 'Should redirect to the page associated with the persistent ID on form submission.' ); From fb3647f26f8cf879d7afd36aeb524f9cab506ba1 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 20:03:33 -0400 Subject: [PATCH 08/10] Fix failing test --- ...ecialPersistentPageIdentifierResolverIntegrationTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php index 653bc45..29d8a1c 100644 --- a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php +++ b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php @@ -56,7 +56,7 @@ public function testExecuteWithNonExistentIdReturns(): void { $resolver = $this->newSpecialPage(); // Call parent::execute first before attempting to execute with an invalid subpage // This is only needed for the test - // $resolver->getContext()->setTitle( $resolver->getPageTitle() ); + $resolver->getContext()->setTitle( $resolver->getPageTitle() ); $resolver->execute( 'non-existent-id' ); $this->assertSame( @@ -70,7 +70,7 @@ public function testExecuteWithEmptySubPageReturns(): void { $resolver = $this->newSpecialPage(); // Call parent::execute first before attempting to execute with an empty subpage // This is only needed for the test - // $resolver->getContext()->setTitle( $resolver->getPageTitle() ); + $resolver->getContext()->setTitle( $resolver->getPageTitle() ); $resolver->execute( '' ); $this->assertSame( @@ -84,7 +84,7 @@ public function testExecuteWithNullSubPageReturns(): void { $resolver = $this->newSpecialPage(); // Call parent::execute first before attempting to execute with a null subpage // This is only needed for the test - // $resolver->getContext()->setTitle( $resolver->getPageTitle() ); + $resolver->getContext()->setTitle( $resolver->getPageTitle() ); $resolver->execute( null ); $this->assertSame( From 97b39bfa8fc7dbc2fa5c87d27b54dda51da76b19 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 20:16:30 -0400 Subject: [PATCH 09/10] Ensure persistent IDs are generated for existing pages in GenerateMissingIdentifiersTest. --- ...cialPersistentPageIdentifierResolverIntegrationTest.php | 7 +++---- tests/Maintenance/GenerateMissingIdentifiersTest.php | 5 +++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php index 29d8a1c..820a976 100644 --- a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php +++ b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php @@ -18,7 +18,6 @@ class SpecialPersistentPageIdentifierResolverIntegrationTest extends PersistentPageIdentifiersIntegrationTest { private PersistentPageIdentifiersRepo $repo; - private WikiPage $page; protected function setUp(): void { parent::setUp(); @@ -40,13 +39,13 @@ protected function newSpecialPage(): SpecialPersistentPageIdentifierResolver { } public function testExecuteWithValidIdRedirects(): void { - $this->page = $this->createPageWithText(); + $page = $this->createPageWithText(); $resolver = $this->newSpecialPage(); - $resolver->execute( $this->repo->getPersistentId( $this->page->getId() ) ); + $resolver->execute( $this->repo->getPersistentId( $page->getId() ) ); $this->assertSame( - $this->page->getTitle()->getFullURL(), + $page->getTitle()->getFullURL(), $resolver->getOutput()->getRedirect(), 'Should redirect to the page associated with the persistent ID.' ); diff --git a/tests/Maintenance/GenerateMissingIdentifiersTest.php b/tests/Maintenance/GenerateMissingIdentifiersTest.php index cad1f99..ff1d58d 100644 --- a/tests/Maintenance/GenerateMissingIdentifiersTest.php +++ b/tests/Maintenance/GenerateMissingIdentifiersTest.php @@ -21,6 +21,11 @@ protected function setUp(): void { $this->maintenance = new GenerateMissingIdentifiers(); $this->maintenance->checkRequiredExtensions(); + + // Run first to ensure that all existing pages have persistent IDs + // This is required because the pages persist into GenerateMissingIdentifiersTest in + // older MediaWiki/PHPUnit version. This is not needed for MW 1.42+. + $this->maintenance->execute(); } protected function tearDown(): void { From b805080a7c2ec780cbbfbf3f83341c0c3b23c6fc Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Tue, 6 May 2025 20:19:54 -0400 Subject: [PATCH 10/10] Remove unnecessary tearDown method from SpecialPersistentPageIdentifierResolverIntegrationTest --- ...alPersistentPageIdentifierResolverIntegrationTest.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php index 820a976..2ee9a68 100644 --- a/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php +++ b/tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php @@ -25,15 +25,6 @@ protected function setUp(): void { $this->repo = new DatabasePersistentPageIdentifiersRepo( $this->db ); } - protected function tearDown(): void { - // This is required because the pages persist into GenerateMissingIdentifiersTest in - // older MediaWiki/PHPUnit version. This is not needed for MW 1.42+. - if ( isset( $this->page ) ) { - $this->deletePage( $this->page ); - } - parent::tearDown(); - } - protected function newSpecialPage(): SpecialPersistentPageIdentifierResolver { return new SpecialPersistentPageIdentifierResolver(); }