Add Special:PersistentPageIdentifierResolver page #64
Conversation
📝 WalkthroughWalkthroughThis change introduces a new special page, "PersistentPageIdentifierResolver", which allows resolving persistent page identifiers to MediaWiki page URLs. It adds the resolver class, registers the special page and its alias, updates localization files, and extends the persistent page identifier repository interface and its implementations with a reverse lookup method. Comprehensive tests are included for the new resolver functionality. Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
tests/Adapters/PageIdsRepoTest.php (2)
20-20:⚠️ Potential issueFix deprecated dynamic property creation.
The pipeline shows a deprecation warning about creating the dynamic property
$tablesUsed. This property should be declared at the class level.class PageIdsRepoTest extends PersistentPageIdentifiersIntegrationTest { private PageIdsRepo $repo; + protected array $tablesUsed = []; protected function setUp(): void { parent::setUp(); $this->tablesUsed[] = 'page'; $this->tablesUsed[] = 'persistent_page_ids'; $this->repo = new PageIdsRepo( $this->db ); }🧰 Tools
🪛 GitHub Actions: CI
[warning] 20-20: PHP Deprecated: Creation of dynamic property ProfessionalWiki\PersistentPageIdentifiers\Tests\Adapters\PageIdsRepoTest::$tablesUsed is deprecated.
[error] Multiple test errors due to missing class "Title" causing failures in tests such as testReturnsAnEmptyArrayWhenThereAreNoPagesWithPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIds, testReturnsPageIdsOnlyForPagesWithoutPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIdsUpToLimit, and testGetPageIdFromPersistentId.
1-91: 💡 Verification agent🧩 Analysis chain
Address Title class dependency failures.
The pipeline indicates multiple test failures due to the missing "Title" class. This suggests a dependency issue in the test environment.
🏁 Script executed:
#!/bin/bash # Check if the Title class is being properly loaded in tests # Look for test bootstrap files that might need to include MediaWiki's Title class fd --type f --search-path './tests' -e php | grep -i bootstrap | xargs grep -l "Title" || echo "No Title class imports found in bootstrap files" # Check how Title is being used in other tests that might be working fd --type f --search-path './tests' -e php | xargs grep -l "use.*Title" | head -n 3 | xargs catLength of output: 1246
Ensure MediaWiki core classes are loaded in your test bootstrap
The failures for
Titleindicate that your tests aren’t pulling in MediaWiki’s core autoloader before running. You’ll need to update your PHPUnit bootstrap so that global classes likeTitleandWikiPageare available.• Create or update
tests/bootstrap.phpto include MediaWiki’s autoload. For example:<?php // tests/bootstrap.php // Adjust this path to point at your MediaWiki root require_once __DIR__ . '/../vendor/autoload.php'; require_once __DIR__ . '/../../tests/MediaWikiIntegrationTestCase.php';• In your
phpunit.xml.dist, ensure this bootstrap is referenced:<phpunit bootstrap="tests/bootstrap.php" …>After adding MediaWiki’s autoloader in
tests/bootstrap.php(and referencing it inphpunit.xml.dist), rerun the pipeline—Titleand other core classes will resolve correctly.🧰 Tools
🪛 GitHub Actions: CI
[warning] 20-20: PHP Deprecated: Creation of dynamic property ProfessionalWiki\PersistentPageIdentifiers\Tests\Adapters\PageIdsRepoTest::$tablesUsed is deprecated.
[error] Multiple test errors due to missing class "Title" causing failures in tests such as testReturnsAnEmptyArrayWhenThereAreNoPagesWithPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIds, testReturnsPageIdsOnlyForPagesWithoutPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIdsUpToLimit, and testGetPageIdFromPersistentId.
🧹 Nitpick comments (4)
src/Adapters/PageIdsRepo.php (1)
33-42: Consider adding PHPDoc to the methodAdding PHPDoc would improve code documentation and provide better type hints for IDEs.
+/** + * Get the page ID corresponding to a persistent ID. + * + * @param string $persistentId The persistent unique identifier to look up + * @return int|null The page ID if found, null otherwise + */ public function getPageIdFromPersistentId( string $persistentId ): ?int {🧰 Tools
🪛 GitHub Check: PHPStan: MW REL1_39, PHP 8.2
[failure] 41-41:
Cannot access property $page_id on stdClass|true.src/EntryPoints/SpecialPURIResolver.php (3)
34-42: Add type information to getFormFields return value.The static analyzer identified missing type information in the return value of getFormFields.
- protected function getFormFields() { + protected function getFormFields(): array { return [ 'puri' => [ 'type' => 'text', 'label-message' => 'puriresolver-puri', 'required' => true, ] ]; }🧰 Tools
🪛 GitHub Check: PHPStan: MW REL1_39, PHP 8.2
[failure] 34-34:
Method ProfessionalWiki\PersistentPageIdentifiers\EntryPoints\SpecialPURIResolver::getFormFields() return type has no value type specified in iterable type array.
63-71: Consider adding documentation for the getTitleFromPersistentId method.This helper method is a key component of the resolver functionality and would benefit from brief documentation.
+ /** + * Converts a persistent ID to a Title object + * + * @param string $persistentId The persistent identifier to resolve + * @return Title|null Title object if found, null otherwise + */ private function getTitleFromPersistentId( string $persistentId ): ?Title { $pageId = $this->getPageIdFromPersistentId( $persistentId ); if ( $pageId ) { return Title::newFromID( $pageId ); } return null; }
73-75: Consider adding documentation for the getPageIdFromPersistentId method.This helper method is a key component of the resolution process and would benefit from brief documentation.
+ /** + * Gets the page ID associated with a persistent ID + * + * @param string $persistentId The persistent identifier to lookup + * @return int|null Page ID if found, null otherwise + */ private function getPageIdFromPersistentId( string $persistentId ): ?int { return PersistentPageIdentifiersExtension::getInstance()->getPageIdsRepo()->getPageIdFromPersistentId( $persistentId ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
PersistentPageIdentifiers.alias.php(1 hunks)extension.json(1 hunks)i18n/en.json(1 hunks)i18n/qqq.json(1 hunks)src/Adapters/PageIdsRepo.php(1 hunks)src/EntryPoints/SpecialPURIResolver.php(1 hunks)tests/Adapters/PageIdsRepoTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Adapters/PageIdsRepo.php (1)
src/EntryPoints/SpecialPURIResolver.php (1)
getPageIdFromPersistentId(73-75)
tests/Adapters/PageIdsRepoTest.php (3)
tests/PersistentPageIdentifiersIntegrationTest.php (1)
createPageWithText(13-19)src/EntryPoints/SpecialPURIResolver.php (1)
getPageIdFromPersistentId(73-75)src/Adapters/PageIdsRepo.php (1)
getPageIdFromPersistentId(33-42)
🪛 GitHub Check: PHPStan: MW REL1_39, PHP 8.2
src/Adapters/PageIdsRepo.php
[failure] 41-41:
Cannot access property $page_id on stdClass|true.
src/EntryPoints/SpecialPURIResolver.php
[failure] 34-34:
Method ProfessionalWiki\PersistentPageIdentifiers\EntryPoints\SpecialPURIResolver::getFormFields() return type has no value type specified in iterable type array.
[failure] 44-44:
Method ProfessionalWiki\PersistentPageIdentifiers\EntryPoints\SpecialPURIResolver::onSubmit() has parameter $data with no value type specified in iterable type array.
[failure] 44-44:
Method ProfessionalWiki\PersistentPageIdentifiers\EntryPoints\SpecialPURIResolver::onSubmit() return type has no value type specified in iterable type array.
[failure] 52-52:
Method ProfessionalWiki\PersistentPageIdentifiers\EntryPoints\SpecialPURIResolver::onSubmit() should return array|bool|Status|string but return statement is missing.
🪛 GitHub Actions: CI
tests/Adapters/PageIdsRepoTest.php
[warning] 20-20: PHP Deprecated: Creation of dynamic property ProfessionalWiki\PersistentPageIdentifiers\Tests\Adapters\PageIdsRepoTest::$tablesUsed is deprecated.
[error] Multiple test errors due to missing class "Title" causing failures in tests such as testReturnsAnEmptyArrayWhenThereAreNoPagesWithPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIds, testReturnsPageIdsOnlyForPagesWithoutPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIdsUpToLimit, and testGetPageIdFromPersistentId.
🔇 Additional comments (9)
PersistentPageIdentifiers.alias.php (1)
1-7: Implementation looks goodThe special page alias for "PURIResolver" is correctly defined following MediaWiki's conventions for extension alias files.
i18n/en.json (1)
11-14: Localization strings properly addedThe new localization strings for the PURI resolver functionality have been correctly added, and the necessary comma has been added to fix the JSON syntax.
i18n/qqq.json (1)
10-13: Documentation strings properly addedThe documentation for the new localization strings is clear and follows the standard format. The comma fix maintains valid JSON structure.
extension.json (2)
64-66: Configuration for SpecialPURIResolver looks good.The registration of the new special page is correctly configured, linking the name "PURIResolver" to the implementation class.
69-69: Alias registration is properly configured.The alias file for the new special page has been correctly added to the ExtensionMessagesFiles section.
tests/Adapters/PageIdsRepoTest.php (2)
74-85: Test for retrieving page ID from persistent ID looks good.The test creates a page, retrieves its persistent ID directly from the database, and verifies that the repository can resolve the persistent ID back to the correct page ID.
🧰 Tools
🪛 GitHub Actions: CI
[error] Multiple test errors due to missing class "Title" causing failures in tests such as testReturnsAnEmptyArrayWhenThereAreNoPagesWithPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIds, testReturnsPageIdsOnlyForPagesWithoutPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIdsUpToLimit, and testGetPageIdFromPersistentId.
87-89: Test for non-existent persistent ID is appropriately implemented.The test correctly verifies that
nullis returned when trying to retrieve a page ID from a non-existent persistent ID.🧰 Tools
🪛 GitHub Actions: CI
[error] Multiple test errors due to missing class "Title" causing failures in tests such as testReturnsAnEmptyArrayWhenThereAreNoPagesWithPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIds, testReturnsPageIdsOnlyForPagesWithoutPersistentIds, testReturnsPageIdsForPagesWithoutPersistentIdsUpToLimit, and testGetPageIdFromPersistentId.
src/EntryPoints/SpecialPURIResolver.php (2)
12-17: Special page class looks well-structured.The special page class properly extends FormSpecialPage and is correctly initialized with the appropriate name.
18-32: The execute method is implemented correctly.The subpage handling works appropriately - when a PURI is provided as a subpage, it attempts to resolve it and redirect if successful. If no matching page is found, it defaults to showing the form.
alistair3149
force-pushed
the
feat-resolver
branch
3 times, most recently
from
7a82b06 to
08c60cd
Compare
alistair3149
force-pushed
the
feat-resolver
branch
from
08c60cd to
26f4f49
Compare
|
Some thoughts here:
It's possible the above is just nitpicking. I'm aware that the format can change, but the ID cannot, so it makes sense to limit this lookup to the saved ID. |
|
Let's mark MW 1.44/master CI as experimental. Or just remove it for now. The README already only mentions 1.39-1.43 support. Then we should also mention it explicitly on our documentation page. |
|
Yes, master CI should not be voting if it is broken |
|
Behavior looks mostly good, some issues with the code. Would be nice to have an integration test for the special page that tests the happy path Morne's point about the PURI terminology made me think that it would be good to left-strip the prefix so the form takes both the full URL (or whatever the formatted version is) and the core ID. Though AFAIK the "core ID" is not exposed anywhere, and basically an internal concept in the extension. So it's more important for the form to be able to handle the formatted version. Definitly no need to do the left-strip behavior in this PR, and it's not quite as simple as a left-strip, since the config is not purely a prefix, so the transformation is a bit more complex. Edit: then again, we do need that extraction logic anyway to handle people navigating to For users who want to use the resolver page, a sensible config is $wgPersistentPageIdentifiersFormat = 'https://example.com/wiki/Special:PURIResolver/$1'; |
alistair3149
force-pushed
the
feat-resolver
branch
from
5adda66 to
a00731b
Compare
alistair3149
force-pushed
the
feat-resolver
branch
from
a00731b to
d0d79a6
Compare
alistair3149
force-pushed
the
feat-resolver
branch
2 times, most recently
from
273dc47 to
61481e2
Compare
tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/Adapters/DatabasePersistentPageIdentifiersRepo.php (1)
62-70: Database implementation for reverse lookup method.The implementation correctly queries the database for a page ID matching the given persistent ID and handles the case when no match is found.
Consider adding
caller(__METHOD__)beforefetchField()for consistency with thegetPersistentIdsmethod:public function getPageIdFromPersistentId( string $persistentId ): ?int { $pageId = $this->database->newSelectQueryBuilder() ->select( 'page_id' ) ->from( 'persistent_page_ids' ) ->where( [ 'persistent_id' => $persistentId ] ) + ->caller( __METHOD__ ) ->fetchField(); return is_numeric( $pageId ) ? (int)$pageId : null; }src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
18-26: Optimize redirect URL lookup.The
getRedirectUrl()method is called twice, which could lead to duplicate database queries.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 ) ); + $redirectUrl = $this->getRedirectUrl( $subPage ); + if ( $redirectUrl !== null ) { + $this->getOutput()->redirect( $redirectUrl ); return; } parent::execute( $subPage ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
PersistentPageIdentifiers.alias.php(1 hunks)extension.json(1 hunks)i18n/en.json(1 hunks)i18n/qqq.json(1 hunks)phpstan-baseline.neon(1 hunks)src/Adapters/DatabasePersistentPageIdentifiersRepo.php(1 hunks)src/Adapters/StubPersistentPageIdentifiersRepo.php(1 hunks)src/Application/PersistentPageIdentifiersRepo.php(1 hunks)src/EntryPoints/SpecialPersistentPageIdentifierResolver.php(1 hunks)tests/Adapters/DatabasePersistentPageIdentifiersRepoTest.php(1 hunks)tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php(1 hunks)tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- i18n/en.json
- phpstan-baseline.neon
- i18n/qqq.json
- PersistentPageIdentifiers.alias.php
- extension.json
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/Application/PersistentPageIdentifiersRepo.php (4)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
getPageIdFromPersistentId(83-85)src/Adapters/DatabasePersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(62-70)src/Adapters/StubPersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(28-30)tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(25-28)
src/Adapters/DatabasePersistentPageIdentifiersRepo.php (4)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
getPageIdFromPersistentId(83-85)src/Adapters/StubPersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(28-30)src/Application/PersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(22-22)tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(25-28)
tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php (4)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
getPageIdFromPersistentId(83-85)src/Adapters/DatabasePersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(62-70)src/Adapters/StubPersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(28-30)src/Application/PersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(22-22)
src/Adapters/StubPersistentPageIdentifiersRepo.php (4)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
getPageIdFromPersistentId(83-85)src/Adapters/DatabasePersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(62-70)src/Application/PersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(22-22)tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php (1)
getPageIdFromPersistentId(25-28)
🪛 GitHub Actions: CI
tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
[error] 46-46: Error: Call to a member function getPrefixedText() on null in HTMLForm.php at line 1260 during testExecuteWithNonExistentIdReturns.
[error] 57-57: Error: Call to a member function getPrefixedText() on null in HTMLForm.php at line 1260 during testExecuteWithEmptySubPageReturns.
[error] 68-68: Error: Call to a member function getPrefixedText() on null in HTMLForm.php at line 1260 during testExecuteWithNullSubPageReturns.
🔇 Additional comments (11)
src/Application/PersistentPageIdentifiersRepo.php (1)
22-22: Interface method added for reverse lookup functionality.The new method
getPageIdFromPersistentIdcomplements the existing functionality by providing reverse lookup capability. This enables resolving persistent identifiers to page IDs, which is essential for the new resolver special page.src/Adapters/StubPersistentPageIdentifiersRepo.php (1)
28-30: Stub implementation of the reverse lookup method.This stub implementation correctly conforms to the interface signature but doesn't actually use the input parameter (
$persistentId). This is acceptable for a stub class whose purpose is to provide test values rather than actual logic.tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php (1)
25-28: Test double implementation for reverse lookup.Good implementation using
array_searchwith strict comparison to find the page ID from the persistent ID. The method properly handles the case when no match is found by returningnullwhenarray_searchreturnsfalse.tests/Adapters/DatabasePersistentPageIdentifiersRepoTest.php (2)
105-111: LGTM! Proper test coverage for the new getPageIdFromPersistentId method.The test effectively verifies that after saving a mapping, the repository correctly returns the original page ID when queried with the persistent ID.
113-115: LGTM! Good error case coverage.This test correctly verifies that the method returns null when queried with a non-existent persistent ID.
tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php (4)
1-29: LGTM! Good test setup and structure.The test class is properly set up with the necessary database tables and repository configuration.
31-42: LGTM! Test covers the happy path correctly.The test verifies that when a valid persistent ID is provided, the special page correctly redirects to the associated page.
77-89: LGTM! Form submission test is robust.This test correctly verifies that submitting a valid ID through the form redirects to the associated page.
91-115: LGTM! Error handling is well-tested.This test properly verifies that submitting a non-existent ID returns a fatal status with the appropriate error message.
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (2)
12-16: LGTM! Class properly extends FormSpecialPage.The class is correctly set up as a FormSpecialPage with an appropriate name.
83-85: LGTM! Proper delegation to the repository.The method correctly delegates to the persistent page identifiers repository to fetch the page ID.
tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
Show resolved
Hide resolved
tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
Show resolved
Hide resolved
tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
Show resolved
Hide resolved
tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (2)
30-38: 🛠️ Refactor suggestionUse a resolver-specific label for the form field
The form field currently uses 'persistentpageidentifiers-info-label' which appears to be a generic label rather than one specific to this resolver page.
Change the label to a resolver-specific one and add a help message to clarify what formats are accepted:
protected function getFormFields(): array { return [ 'persistentpageidentifier' => [ 'type' => 'text', - 'label-message' => 'persistentpageidentifiers-info-label', + 'label-message' => 'persistentpageidentifierresolver-info-label', + 'help-message' => 'persistentpageidentifierresolver-form-help', 'required' => true, ] ]; }Make sure to add the corresponding messages in the i18n files that explain both raw IDs and formatted PURIs are accepted.
72-80: 🛠️ Refactor suggestionVerify that the Title exists and is accessible
The current implementation checks if the title exists at line 65 after retrieving the URL, but this check should ideally happen when retrieving the Title itself.
Move the existence check to the
getTitleFromPersistentIdmethod:private function getTitleFromPersistentId( string $persistentId ): ?Title { $pageId = $this->getPageIdFromPersistentId( $persistentId ); if ( $pageId !== null ) { - return Title::newFromID( $pageId ); + $title = Title::newFromID( $pageId ); + // Check if the title exists and is accessible + if ( $title !== null && $title->exists() ) { + return $title; + } } return null; }This would make the existence check centralized and remove the redundant check in
getUrlFromPersistentId.
🧹 Nitpick comments (4)
src/Presentation/PersistentPageIdFormatter.php (1)
22-26: Implementation looks good but consider adding additional safeguardsThe extraction logic using regex is well implemented with proper pattern escaping and anchoring. The non-greedy capture and fallback behavior are appropriate.
Consider adding validation for the extracted ID to ensure it matches expected format (like UUID validation if that's your use case). This could prevent issues where an arbitrary string gets extracted from a malformed input:
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; + if ( preg_match( $pattern, $input, $matches ) ) { + // Optional: Add validation for the extracted ID format here + return $matches[1]; + } + return $input; }src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (2)
40-48: Extract ID before lookup in form submission handlingCurrently, the form submission handler passes the raw input to
getUrlFromPersistentId, which then callsextractId. This is good, but not obvious from the function call.Consider extracting the ID explicitly in
onSubmitfor better code clarity:public function onSubmit( array $data ): Status|bool { - $url = $this->getUrlFromPersistentId( $data['persistentpageidentifier'] ); + $persistentId = $this->extractId( $data['persistentpageidentifier'] ); + $url = $this->getUrlFromPersistentId( $persistentId ); if ( $url === null ) { return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } $this->getOutput()->redirect( $url ); return true; }This makes the extraction step more explicit and follows the same pattern as used in the
executemethod with the subpage parameter.
58-70: Simplify getUrlFromPersistentId by removing redundant checksThe method
getUrlFromPersistentIdis doing redundant existence checks that could be handled bygetTitleFromPersistentId.If you implement the suggested changes to
getTitleFromPersistentId, you can simplify this method:private function getUrlFromPersistentId( ?string $persistentId ): ?string { if ( $persistentId === null || $persistentId === '' ) { return null; } $title = $this->getTitleFromPersistentId( $this->extractId( $persistentId ) ); - if ( $title === null || !$title->exists() ) { + if ( $title === null ) { return null; } return $title->getFullURL(); }This makes the code more maintainable by avoiding duplicate existence checks.
tests/Presentation/PersistentPageIdFormatterTest.php (1)
25-33: Tests cover basic functionality but consider adding more edge casesThe added tests verify the core functionality of the new
extractIdmethod. However, more comprehensive testing would be beneficial.Consider adding tests for additional scenarios:
public function testExtractsIdWithSpecialRegexCharacters(): void { $formatter = new PersistentPageIdFormatter( 'https://example.com/id/$1' ); $this->assertSame( '42', $formatter->extractId( 'https://example.com/id/42' ) ); } public function testExtractsIdWithEmptyInput(): void { $formatter = new PersistentPageIdFormatter( 'foo $1 bar' ); $this->assertSame( '', $formatter->extractId( '' ) ); } public function testExtractsIdWithComplexFormat(): void { $formatter = new PersistentPageIdFormatter( 'https://example.com/wiki/Special:PURIResolver/$1' ); $this->assertSame( '0196a147-3cd4-7336-a60a-7ed32140137f', $formatter->extractId( 'https://example.com/wiki/Special:PURIResolver/0196a147-3cd4-7336-a60a-7ed32140137f' ) ); }These additional tests would help ensure the extraction works correctly with real-world URL patterns and edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Application/PersistentPageIdentifiersRepo.php(1 hunks)src/EntryPoints/SpecialPersistentPageIdentifierResolver.php(1 hunks)src/Presentation/PersistentPageIdFormatter.php(1 hunks)tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php(1 hunks)tests/Maintenance/GenerateMissingIdentifiersTest.php(1 hunks)tests/Presentation/PersistentPageIdFormatterTest.php(1 hunks)tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Application/PersistentPageIdentifiersRepo.php
- tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php
- tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/Maintenance/GenerateMissingIdentifiersTest.php (1)
maintenance/GenerateMissingIdentifiers.php (1)
execute(23-42)
src/Presentation/PersistentPageIdFormatter.php (1)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
extractId(86-88)
🔇 Additional comments (2)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
86-88: Good use of the new extractId methodThe method uses the newly added
extractIdmethod fromPersistentPageIdFormattercorrectly, leveraging the extension's instance to get the formatter.tests/Maintenance/GenerateMissingIdentifiersTest.php (1)
24-28: Well-documented workaround for older MediaWiki/PHPUnit versionsThe added execution of the maintenance script during setup ensures consistent test behavior across different MediaWiki versions. The comments clearly explain the rationale.
alistair3149
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (2)
33-35: Consider using resolver-specific message keysThe form currently uses
persistentpageidentifiers-info-labelfor its label, but it would be clearer to use a resolver-specific message key.- 'label-message' => 'persistentpageidentifiers-info-label', + 'label-message' => 'persistentpageidentifierresolver-info-label',
32-37: Add help text to clarify input format expectationsBased on PR comments about confusion between formatted PURIs and core IDs, adding help text would improve the user experience by explicitly stating that both formats are accepted.
'type' => 'text', 'label-message' => 'persistentpageidentifiers-info-label', + 'help-message' => 'persistentpageidentifierresolver-form-help', 'required' => true,You'll need to add appropriate help text in the i18n files explaining that both formats are accepted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Application/PersistentPageIdentifiersRepo.php(1 hunks)src/EntryPoints/SpecialPersistentPageIdentifierResolver.php(1 hunks)src/Presentation/PersistentPageIdFormatter.php(1 hunks)tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php(1 hunks)tests/Maintenance/GenerateMissingIdentifiersTest.php(1 hunks)tests/Presentation/PersistentPageIdFormatterTest.php(1 hunks)tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Application/PersistentPageIdentifiersRepo.php
- tests/TestDoubles/InMemoryPersistentPageIdentifiersRepo.php
- tests/Integration/SpecialPersistentPageIdentifierResolverIntegrationTest.php
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/Maintenance/GenerateMissingIdentifiersTest.php (1)
maintenance/GenerateMissingIdentifiers.php (1)
execute(23-42)
src/Presentation/PersistentPageIdFormatter.php (1)
src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
extractId(86-88)
tests/Presentation/PersistentPageIdFormatterTest.php (2)
src/Presentation/PersistentPageIdFormatter.php (2)
PersistentPageIdFormatter(7-28)extractId(22-26)src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (1)
extractId(86-88)
🔇 Additional comments (5)
src/Presentation/PersistentPageIdFormatter.php (1)
22-26: The ID extraction implementation looks goodThe method correctly handles extracting the ID from a formatted string by creating a regex pattern that captures the value in place of the
$1placeholder. The fallback to returning the original input when no match is found is a sensible approach that allows the method to accept both formatted and raw IDs.src/EntryPoints/SpecialPersistentPageIdentifierResolver.php (2)
1-90: Special page implementation handles both extraction and resolution wellThe implementation correctly resolves persistent page identifiers to their corresponding pages, with proper handling for both subpage execution and form submission. The code also extracts IDs from formatted strings using the
extractIdmethod, addressing the PURI vs. raw ID confusion mentioned in the PR comments.
65-67: Good check for existence and validityThe code properly validates both that the Title object isn't null and that the page exists before returning a URL. This prevents redirects to non-existent pages.
tests/Maintenance/GenerateMissingIdentifiersTest.php (1)
25-28: Good compatibility fix for older MediaWiki versionsThe additional execution in setUp with clear explanatory comments ensures test reliability across different MediaWiki versions. The comment clearly explains why this is necessary for older versions but not for MW 1.42+.
tests/Presentation/PersistentPageIdFormatterTest.php (1)
25-33: Comprehensive tests for the extraction functionalityThese tests effectively verify both cases of the
extractIdmethod:
- When input matches the format pattern, it correctly extracts the ID
- When input doesn't match, it properly returns the original input unchanged
This ensures the extraction behavior works as expected, which is critical for the resolver functionality.
Follow up to #64 @alistair3149 Writing good tests after the fact is more difficult than if you follow the red-green-refactor cycle. You never had the red step for this test, because I could remove the WHERE clause from the query, and it would still pass. To know a test actually does what you want, you need to see it fail first. It's easy to miss testing parts of the production code or to fail to test it altogether due to a mistake in the test.
Follow up to #64 @alistair3149 Writing good tests after the fact is more difficult than if you follow the red-green-refactor cycle. You never had the red step for this test, because I could remove the WHERE clause from the query, and it would still pass. To know a test actually does what you want, you need to see it fail first. It's easy to miss testing parts of the production code or to fail to test it altogether due to a mistake in the test.
Key changes:
Special:PersistentPageIdentifierResolverspecial pageSpecial:PersistentPageIdentifierResolver/0196a147-3cd4-7336-a60a-7ed32140137fredirects to the page with the matching ID, and redirects toSpecial:PersistentPageIdentifierResolverif there are no matchSpecial:PersistentPageIdentifierResolverRecording.2025-05-05.135737.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Tests