Fix/829 parser not properly initialised

[bertrandgorge]

Fixes #829

Summary by CodeRabbit

• Refactor
  • Standardized parser imports across the codebase to use MediaWiki's official namespace conventions.
  • Improved parser initialization with lazy loading and proper context configuration for enhanced reliability.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR updates import statements across 14 files to use the MediaWiki-namespaced Parser class (MediaWiki\Parser\Parser) instead of a global namespace Parser. Additionally, src/SemanticMW/QueryHandler.php introduces lazy initialization of a Parser instance with proper configuration and external parse context setup to address initialization issues.

Changes

Cohort / File(s)	Summary
Parser Import Migration src/Map/CargoFormat/CargoFormat.php, src/Map/DisplayMap/DisplayMapFunction.php, src/Map/DisplayMap/DisplayMapRenderer.php, src/Map/SemanticFormat/MapPrinter.php, src/MapsFactory.php, src/MapsSetup.php, src/ParserHookSetup.php, src/ParserHooks/CoordinatesFunction.php, src/ParserHooks/DistanceFunction.php, src/ParserHooks/FindDestinationFunction.php, src/ParserHooks/GeoDistanceFunction.php, src/ParserHooks/GeocodeFunction.php, src/ParserHooks/MapsDocFunction.php, src/Presentation/WikitextParser.php	Updated use statements to import Parser from MediaWiki\Parser\Parser namespace instead of global namespace, affecting type-hint resolution across all files.
Parser Initialization Enhancement src/SemanticMW/QueryHandler.php	Added imports for RequestContext, Parser, and ParserOptions. Introduced private $parser member with lazy initialization via ParserFactory. Ensures Parser instance is configured with ParserOptions and external parse context via startExternalParse(). Changed getParser() return type from \Parser to Parser and updated implementation to return the initialized instance.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

• src/SemanticMW/QueryHandler.php requires closer attention due to the introduction of lazy initialization logic, private parser member, and return type changes. Verify that the ParserFactory instantiation, ParserOptions initialization, and startExternalParse() call sequence properly address the initialization issue described in the linked issue.
• The remaining 14 files contain repetitive, homogeneous import statement updates with minimal risk.

Possibly related PRs

• Use classes namespaced in MW 1.40 after removal #810: Performs the same namespace migration for global class imports to MediaWiki\*-namespaced classes, including Parser class refactoring across multiple files.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Fix/829 parser not properly initialised" directly addresses the main objective of the changeset. The title clearly references the issue being fixed (829) and describes the core problem: parser initialization. The title is concise, specific, and provides enough context for someone reviewing the commit history to understand that this is a fix for a parser initialization issue without being verbose or vague.
Linked Issues Check	✅ Passed	The code changes directly satisfy the requirements from issue #829. The pull request addresses the core problem by implementing the proposed fix: calling startExternalParse to properly initialize the Parser's typed properties before use. In src/SemanticMW/QueryHandler.php, the changes introduce lazy initialization of a private Parser instance via ParserFactory, ensure ParserOptions are set up correctly, and call startExternalParse before the Parser is used. Additionally, the import path changes across multiple files from use Parser; to use MediaWiki\\Parser\\Parser; align with MediaWiki 1.43 requirements. These changes collectively eliminate the uninitialized property access error that was occurring at line 440.
Out of Scope Changes Check	✅ Passed	All code changes in the pull request are directly related to fixing issue #829. The import path updates across multiple files are necessary for MediaWiki 1.43 compatibility and are part of addressing the root cause of the parser initialization problem. The core logic changes in src/SemanticMW/QueryHandler.php implement the proposed fix by properly initializing the Parser and its options. No unrelated or tangential changes are present in the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6c81636 and efb6992.

📒 Files selected for processing (15)

• src/Map/CargoFormat/CargoFormat.php (1 hunks)
• src/Map/DisplayMap/DisplayMapFunction.php (1 hunks)
• src/Map/DisplayMap/DisplayMapRenderer.php (1 hunks)
• src/Map/SemanticFormat/MapPrinter.php (1 hunks)
• src/MapsFactory.php (1 hunks)
• src/MapsSetup.php (1 hunks)
• src/ParserHookSetup.php (1 hunks)
• src/ParserHooks/CoordinatesFunction.php (1 hunks)
• src/ParserHooks/DistanceFunction.php (1 hunks)
• src/ParserHooks/FindDestinationFunction.php (1 hunks)
• src/ParserHooks/GeoDistanceFunction.php (1 hunks)
• src/ParserHooks/GeocodeFunction.php (1 hunks)
• src/ParserHooks/MapsDocFunction.php (1 hunks)
• src/Presentation/WikitextParser.php (1 hunks)
• src/SemanticMW/QueryHandler.php (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/SemanticMW/QueryHandler.php (1)

src/Map/SemanticFormat/MapPrinter.php (1)

• getParser (68-70)

🔇 Additional comments (17)

src/ParserHooks/MapsDocFunction.php (1)

10-10: LGTM! Namespace migration to MediaWiki\Parser\Parser.

The import update correctly migrates to the namespaced Parser class, maintaining compatibility with MediaWiki 1.43+. The type hint at line 26 properly resolves to this import.

src/ParserHooks/DistanceFunction.php (1)

9-9: LGTM! Consistent namespace migration.

The Parser import update aligns with the broader codebase migration to MediaWiki\Parser\Parser.

src/Map/CargoFormat/CargoFormat.php (1)

9-9: LGTM! Import update maintains compatibility.

The namespace migration correctly updates the Parser reference used in the instanceof check at line 50.

src/Map/SemanticFormat/MapPrinter.php (1)

20-20: LGTM! Parser import correctly updated.

The namespace migration properly supports the return type hints in getParser() and getParserClone() methods.

src/ParserHooks/FindDestinationFunction.php (1)

11-11: LGTM! Import update applied correctly.

src/Map/DisplayMap/DisplayMapFunction.php (1)

15-15: LGTM! Namespace update supports multiple method signatures.

The updated import correctly resolves Parser references in getMapHtmlForKeyValueStrings() and getMapHtmlForParameterList().

src/ParserHookSetup.php (1)

14-14: LGTM! Import update supports property, constructor, and closures.

The namespace migration correctly updates all Parser references including the property at line 20, constructor parameter at line 23, closure parameters, and the constant reference at line 49.

src/ParserHooks/GeocodeFunction.php (1)

10-10: LGTM! Final import update completes the namespace migration.

src/Presentation/WikitextParser.php (1)

7-7: LGTM: Parser namespace migration

The import update aligns with the MediaWiki-namespaced Parser class used across the codebase. No behavioral changes.

src/MapsSetup.php (1)

20-20: LGTM: Parser namespace migration

The import update correctly aligns with the MediaWiki\Parser\Parser namespace used in the closure parameter type at line 64.

src/Map/DisplayMap/DisplayMapRenderer.php (1)

17-17: LGTM: Parser namespace migration

The import update aligns with the MediaWiki-namespaced Parser class. The type hint at line 47 now correctly resolves to MediaWiki\Parser\Parser.

src/ParserHooks/GeoDistanceFunction.php (1)

10-10: LGTM: Parser namespace migration

The import update correctly aligns with the MediaWiki\Parser\Parser namespace. The type hint at line 23 now resolves to the correct namespaced class.

src/MapsFactory.php (1)

41-41: LGTM: Parser namespace migration

The import update aligns with the MediaWiki-namespaced Parser class used in the method signature at line 277.

src/ParserHooks/CoordinatesFunction.php (1)

9-9: LGTM: Parser namespace migration

The import update correctly aligns with the MediaWiki\Parser\Parser namespace used in the handle() method at line 22.

src/SemanticMW/QueryHandler.php (3)

10-12: LGTM: Required imports for Parser initialization

The new imports support the Parser initialization fix. These are necessary for creating ParserOptions and obtaining the current user context.

---

48-48: LGTM: Lazy Parser initialization

Adding the private $parser property enables lazy initialization and reuse within the same query context, which is appropriate for this use case.

---

475-504: Now I need to examine the actual code in QueryHandler to understand the template parsing flow:

Now let me search the repository to examine the actual QueryHandler code more carefully:

Based on my research, I now have sufficient information to provide a comprehensive rewritten review comment.

Verify null Title handling and remove redundant state clearing

The parser requires a Title object as input, but passing null to startExternalParse() is technically allowed since the parameter is nullable. However, when setTitle() receives null, it auto-creates a "Badtitle/Parser" Title, which means template resolution will occur in the wrong context. This could affect templates used in semantic queries, particularly if they depend on page-relative links or page-specific behavior.

Additionally, line 493 explicitly calls clearState(), and then line 500 calls startExternalParse(..., true) which calls clearState() again through startParse(). clearState() is expensive (~10ms) so multiple calls should be avoided if possible. Remove the explicit clearState() call at line 493 or pass false to startExternalParse() to avoid the redundancy.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull Request Overview

This PR updates the codebase to use the namespaced MediaWiki\Parser\Parser class instead of the global \Parser class. The main functional change is in QueryHandler.php, which now properly initializes a Parser instance from ParserFactory and sets it up for external parsing with startExternalParse().

• Updated imports across 14 files to use the namespaced MediaWiki\Parser\Parser class
• Refactored QueryHandler::getParser() to create a fresh Parser instance with proper initialization
• Added parser instance caching in QueryHandler to reuse the same parser for related parsing operations

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/SemanticMW/QueryHandler.php	Refactored parser initialization to use ParserFactory, cache parser instance, and call startExternalParse()
src/Presentation/WikitextParser.php	Updated Parser import to use namespaced class
src/ParserHooks/MapsDocFunction.php	Updated Parser import to use namespaced class
src/ParserHooks/GeocodeFunction.php	Updated Parser import to use namespaced class
src/ParserHooks/GeoDistanceFunction.php	Updated Parser import to use namespaced class
src/ParserHooks/FindDestinationFunction.php	Updated Parser import to use namespaced class
src/ParserHooks/DistanceFunction.php	Updated Parser import to use namespaced class
src/ParserHooks/CoordinatesFunction.php	Updated Parser import to use namespaced class
src/ParserHookSetup.php	Updated Parser import to use namespaced class
src/MapsSetup.php	Updated Parser import to use namespaced class
src/MapsFactory.php	Updated Parser import to use namespaced class
src/Map/SemanticFormat/MapPrinter.php	Updated Parser import to use namespaced class
src/Map/DisplayMap/DisplayMapRenderer.php	Updated Parser import to use namespaced class
src/Map/DisplayMap/DisplayMapFunction.php	Updated Parser import to use namespaced class
src/Map/CargoFormat/CargoFormat.php	Updated Parser import to use namespaced class

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Inconsistent indentation in comment. Lines 477-478 use spaces instead of tabs for leading whitespace, while line 476 uses a tab.

Suggested change

	* * Build a fresh Parser from ParserFactory->create() rather than the global shared instance.
	* * Reuse that initialized Parser for all related parsing that belongs to the same context Title and Options.
	* * Build a fresh Parser from ParserFactory->create() rather than the global shared instance.
	* * Reuse that initialized Parser for all related parsing that belongs to the same context Title and Options.

[Copilot]

The first parameter to startExternalParse() is null, which means the parser has no Title context. This could cause issues with features that depend on page context (like template transclusion, relative links, or page-dependent variables). Consider passing an appropriate Title object if available, or document why null is acceptable in this context.

[JeroenDeDauw]

Looks reasonable. Test failures seem the same as on master. Did you manually test this? Some details on version and tests done is useful.

[bertrandgorge]

Hello @JeroenDeDauw - yes, it's even on production on Triple Performance with this set of versions (I didn't test on other versions though):

Maps version: 13.0.0-alpha (setup with composer: 'mediawiki/maps "^12.0"')
MW version: 1.43.5
PHP version: 8.2.29
SMW version : 6.0.1