Fix "Call to undefined method MediaWiki\Parser\ParserOutput::getText()"#837
Fix "Call to undefined method MediaWiki\Parser\ParserOutput::getText()"#837paladox wants to merge 5 commits intoProfessionalWiki:masterfrom
Conversation
|
Fixes MW 1.45 support (well one issue) |
📝 WalkthroughWalkthroughReworks extraction in parser result handling: captures parser output into a variable, returns Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Presentation/WikitextParser.php (1)
39-40: Adding explicit parameter togetText()is optional but harmless.According to MediaWiki 1.39 documentation,
ParserOutput::getText()has a default parameter ($options = []), so calling without arguments is safe and should not cause warnings. The codebase's defensive approach usingmethod_exists()checks appropriately handles version differences by prioritizinggetContentHolderText()andgetRawText()before falling back togetText(). Other calls togetText()in the repository (e.g.,tests/Util/TestFactory.php,tests/System/SemanticQueryTest.php) follow the same pattern without explicit parameters.If you prefer to be more explicit:
Optional improvement
} elseif ( method_exists( $parserOutput, 'getText' ) ) { - return $parserOutput->getText(); + return $parserOutput->getText( [] ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Presentation/WikitextParser.php` around lines 39 - 40, The fallback call to ParserOutput::getText() in WikitextParser (the elseif branch that checks method_exists($parserOutput, 'getText')) should be made explicit by passing the default options parameter to avoid any ambiguity; change the return from $parserOutput->getText() to $parserOutput->getText([]) so it explicitly supplies the default $options = [] when calling ParserOutput::getText().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Presentation/WikitextParser.php`:
- Around line 39-40: The fallback call to ParserOutput::getText() in
WikitextParser (the elseif branch that checks method_exists($parserOutput,
'getText')) should be made explicit by passing the default options parameter to
avoid any ambiguity; change the return from $parserOutput->getText() to
$parserOutput->getText([]) so it explicitly supplies the default $options = []
when calling ParserOutput::getText().
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Presentation/WikitextParser.php (1)
30-36: ConsiderhasText()guard instead of exception-as-control-flow, and log unexpectedLogicExceptions.
ParserOutputis documented to represent "no HTML output" with null and throw ongetRawText()in that case — which is what happens for redirect pages that do not generate HTML output. The current catch silently swallows the exception without any diagnostic trace, making it hard to distinguish a legitimately empty parse from an unexpected failure. Consider the following improvements:♻️ Suggested improvement
- if ( method_exists( $parserOutput, 'getContentHolderText' ) ) { - try { - return $parserOutput->getContentHolderText(); - } catch ( LogicException $e ) { - // Handle case where there is no body content - return ''; - } - } elseif ( method_exists( $parserOutput, 'getText' ) ) { + if ( method_exists( $parserOutput, 'getContentHolderText' ) ) { + // hasText() avoids exception-as-control-flow for the "no HTML" case + if ( method_exists( $parserOutput, 'hasText' ) && !$parserOutput->hasText() ) { + return ''; + } + try { + return $parserOutput->getContentHolderText(); + } catch ( LogicException $e ) { + // Unexpected: log for diagnostics but degrade gracefully + wfLogWarning( 'WikitextParser: getContentHolderText() threw: ' . $e->getMessage() ); + return ''; + } + } elseif ( method_exists( $parserOutput, 'getText' ) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Presentation/WikitextParser.php` around lines 30 - 36, Replace the exception-as-control-flow pattern by first guarding with hasText() on $parserOutput (if method_exists($parserOutput, 'hasText') && !$parserOutput->hasText() return '') and only call getContentHolderText() when text is present; keep a try/catch around getContentHolderText() but in the catch (LogicException $e) call your logger (e.g. $this->logger->warning or similar) to record the unexpected exception before returning an empty string so legitimate empty parses are handled without silencing unexpected failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Presentation/WikitextParser.php`:
- Around line 30-36: Replace the exception-as-control-flow pattern by first
guarding with hasText() on $parserOutput (if method_exists($parserOutput,
'hasText') && !$parserOutput->hasText() return '') and only call
getContentHolderText() when text is present; keep a try/catch around
getContentHolderText() but in the catch (LogicException $e) call your logger
(e.g. $this->logger->warning or similar) to record the unexpected exception
before returning an empty string so legitimate empty parses are handled without
silencing unexpected failures.
|
With the updated CI, it looks like there are still some places where the missing method is used. |
Refactor parse method to handle different parser output methods.
Fixed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Util/TestFactory.php (1)
25-44: Extraction logic is duplicated acrossWikitextParserandTestFactoryThe identical
getContentHolderText/getText/''fallback chain now exists verbatim in bothWikitextParser::wikitextToHtml()andTestFactory::parse(). If the MW API changes again, both copies need to be updated in sync.Consider extracting it to a shared static helper (e.g.,
ParserOutputHelper::getHtmlText(ParserOutput $output): string) consumable by both production and test code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Util/TestFactory.php` around lines 25 - 44, The parsing result extraction logic duplicated between TestFactory::parse and WikitextParser::wikitextToHtml should be centralized: create a shared static helper (e.g., ParserOutputHelper::getHtmlText(ParserOutput $output): string) that implements the existing chain (use getContentHolderText() with LogicException handling, fall back to getText(), then ''), then replace the duplicate code in TestFactory::parse and WikitextParser::wikitextToHtml to call ParserOutputHelper::getHtmlText($parserOutput); ensure type hints/imports match ParserOutput and preserve the existing behavior and exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Util/TestFactory.php`:
- Around line 33-39: The catch currently references an unqualified
LogicException inside the Maps\Tests\Util namespace so thrown global
\LogicException instances won't be caught; update the file so the catch matches
the global exception by either adding a top-level import `use LogicException;`
to the file or changing the catch to `catch (\LogicException $e)` for the block
surrounding parserOutput->getContentHolderText() to ensure the empty-string
fallback runs when getContentHolderText() throws.
---
Nitpick comments:
In `@tests/Util/TestFactory.php`:
- Around line 25-44: The parsing result extraction logic duplicated between
TestFactory::parse and WikitextParser::wikitextToHtml should be centralized:
create a shared static helper (e.g.,
ParserOutputHelper::getHtmlText(ParserOutput $output): string) that implements
the existing chain (use getContentHolderText() with LogicException handling,
fall back to getText(), then ''), then replace the duplicate code in
TestFactory::parse and WikitextParser::wikitextToHtml to call
ParserOutputHelper::getHtmlText($parserOutput); ensure type hints/imports match
ParserOutput and preserve the existing behavior and exception handling.
3 participants
Copies something similar to https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/src/Utils/ParserOutputHelper.php#L29
Summary by CodeRabbit
Bug Fixes
Tests