Skip to content

feat: implement ParserCache for caching parser instances and add cache statistics#22

Merged
konradmichalik merged 4 commits intomainfrom
parser-cache
Jul 10, 2025
Merged

feat: implement ParserCache for caching parser instances and add cache statistics#22
konradmichalik merged 4 commits intomainfrom
parser-cache

Conversation

@konradmichalik
Copy link
Copy Markdown
Contributor

@konradmichalik konradmichalik commented Jul 10, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a caching mechanism for parser instances to improve efficiency.
    • Added display of the number of cached parsers in both CLI and JSON validation result outputs.
    • Extended validation statistics to include the count of cached parsers.
  • Bug Fixes

    • Ensured correct reporting of parser cache statistics in verbose and JSON outputs.
  • Tests

    • Added comprehensive tests for parser caching behavior and statistics reporting.
    • Enhanced tests to cover new statistics fields and output formats.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jul 10, 2025

Warning

Rate limit exceeded

@jackd248 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 59 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between d24e73c and 1988762.

📒 Files selected for processing (4)
  • src/Result/ValidationResultCliRenderer.php (1 hunks)
  • tests/src/PluginExtendedTest.php (1 hunks)
  • tests/src/Result/ValidationResultCliRendererTest.php (4 hunks)
  • tests/src/Utility/PathUtilityExtendedTest.php (1 hunks)

Walkthrough

A static parser caching mechanism was introduced via a new ParserCache class, affecting parser instantiation throughout the codebase. The ParserInterface and its implementations were updated, and cache statistics are now tracked and rendered in CLI and JSON outputs. Comprehensive tests for the cache and statistics were added or updated. The ParserRegistry now supports logging when no parser is found. Plugin lifecycle methods and path normalization utilities received new tests.

Changes

File(s) Change Summary
src/Parser/ParserCache.php Added ParserCache class for static parser instance caching, retrieval, clearing, and statistics.
src/Parser/ParserInterface.php, tests/src/Result/ValidationRunTest.php Added getFileDirectory() method to ParserInterface and mock implementation in test.
src/Validator/AbstractValidator.php, src/Result/ValidationRun.php Parser instantiation now uses ParserCache::get; cache is cleared after use; statistics tracked.
src/Result/ValidationStatistics.php Added parsersCached property and getter to statistics.
src/Result/ValidationResultCliRenderer.php CLI renderer outputs "Parsers cached" statistic in verbose mode.
src/Result/ValidationResultJsonRenderer.php JSON renderer outputs new statistics key with cache and timing data.
src/Parser/ParserRegistry.php Updated resolveParserClass to accept optional logger and log warning if no parser found.
tests/src/Parser/ParserCacheTest.php Added tests for all ParserCache functionality and statistics.
tests/src/Result/ValidationResultCliRendererTest.php Updated tests to check for "Parsers cached" output in verbose and compact modes.
tests/src/Result/ValidationResultJsonRendererTest.php Added tests for statistics rendering and default values in JSON output.
tests/src/Result/ValidationStatisticsTest.php Added and updated tests for parsersCached property and getter in statistics.
tests/src/PluginExtendedTest.php Added tests for Plugin lifecycle methods: activate, deactivate, uninstall.
tests/src/Utility/PathUtilityExtendedTest.php Added tests for PathUtility::normalizeFolderPath covering various path scenarios.

Sequence Diagram(s)

sequenceDiagram
    participant Validator
    participant ParserCache
    participant Parser

    Validator->>ParserCache: get(filePath, parserClass)
    alt Cache hit
        ParserCache-->>Validator: return cached Parser
    else Cache miss
        ParserCache->>Parser: instantiate Parser(filePath)
        ParserCache-->>Validator: return new Parser (cached)
    end
    Validator->>Parser: parse()
    Note over Validator,Parser: After validation
    Validator->>ParserCache: clear()
Loading

Poem

In the warren of code, a cache now appears,
Parsers hop in—no more duplicate peers!
Statistics are counted, results crystal clear,
CLI and JSON now show what’s near.
With tests all around, the garden’s in bloom—
This rabbit approves of the cache in your room!
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
tests/src/Parser/ParserCacheTest.php (1)

88-89: Use consistent class name references.

The code uses inconsistent class name references - YamlParser::class (imported) vs fully qualified \MoveElevator\ComposerTranslationValidator\Parser\XliffParser::class. For consistency, either import XliffParser at the top or use the fully qualified name for both.

Add this import at the top of the file:

 use MoveElevator\ComposerTranslationValidator\Parser\ParserCache;
 use MoveElevator\ComposerTranslationValidator\Parser\YamlParser;
+use MoveElevator\ComposerTranslationValidator\Parser\XliffParser;
 use PHPUnit\Framework\Attributes\CoversClass;

Then update the references:

-            $xliffParser = ParserCache::get($xliffFile, \MoveElevator\ComposerTranslationValidator\Parser\XliffParser::class);
+            $xliffParser = ParserCache::get($xliffFile, XliffParser::class);
-            $this->assertInstanceOf(\MoveElevator\ComposerTranslationValidator\Parser\XliffParser::class, $xliffParser);
+            $this->assertInstanceOf(XliffParser::class, $xliffParser);
tests/src/Result/ValidationStatisticsTest.php (1)

91-103: Consider consolidating duplicate test logic.

The testGetParsersCachedDefault() test duplicates the assertion already present in the testConstructorAndGetters() method (line 30). While having focused tests is good, this creates redundancy.

Consider removing the testGetParsersCachedDefault() method since the default value is already tested in the existing testConstructorAndGetters() method, or provide a clear distinction between the two tests.

-    public function testGetParsersCachedDefault(): void
-    {
-        $statistics = new ValidationStatistics(1.0, 2, 3, 4);
-
-        $this->assertSame(0, $statistics->getParsersCached());
-    }
-
     public function testGetParsersCachedWithValue(): void
     {
         $statistics = new ValidationStatistics(1.0, 2, 3, 4, 5);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7695e60 and 49c1620.

📒 Files selected for processing (12)
  • src/Parser/ParserCache.php (1 hunks)
  • src/Parser/ParserInterface.php (1 hunks)
  • src/Result/ValidationResultCliRenderer.php (1 hunks)
  • src/Result/ValidationResultJsonRenderer.php (2 hunks)
  • src/Result/ValidationRun.php (3 hunks)
  • src/Result/ValidationStatistics.php (2 hunks)
  • src/Validator/AbstractValidator.php (2 hunks)
  • tests/src/Parser/ParserCacheTest.php (1 hunks)
  • tests/src/Result/ValidationResultCliRendererTest.php (4 hunks)
  • tests/src/Result/ValidationResultJsonRendererTest.php (2 hunks)
  • tests/src/Result/ValidationRunTest.php (1 hunks)
  • tests/src/Result/ValidationStatisticsTest.php (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/Result/ValidationResultCliRenderer.php (1)
src/Result/ValidationStatistics.php (1)
  • getParsersCached (47-50)
src/Parser/ParserInterface.php (1)
tests/src/Result/ValidationRunTest.php (1)
  • getFileDirectory (257-260)
tests/src/Result/ValidationRunTest.php (2)
src/Parser/ParserInterface.php (1)
  • getFileDirectory (25-25)
tests/src/Validator/AbstractValidatorTest.php (1)
  • getFileDirectory (95-98)
src/Validator/AbstractValidator.php (2)
src/Parser/ParserCache.php (2)
  • ParserCache (7-36)
  • get (12-21)
src/Parser/ParserRegistry.php (2)
  • ParserRegistry (7-37)
  • resolveParserClass (23-36)
src/Result/ValidationRun.php (3)
src/Parser/ParserCache.php (4)
  • ParserCache (7-36)
  • getCacheStats (29-35)
  • clear (23-26)
  • get (12-21)
src/Result/ValidationStatistics.php (1)
  • ValidationStatistics (7-51)
src/Result/ValidationResult.php (1)
  • ValidationResult (11-69)
tests/src/Parser/ParserCacheTest.php (4)
src/Parser/ParserCache.php (4)
  • ParserCache (7-36)
  • clear (23-26)
  • get (12-21)
  • getCacheStats (29-35)
src/Parser/YamlParser.php (1)
  • YamlParser (9-82)
src/Parser/ParserInterface.php (1)
  • getFilePath (27-27)
src/Parser/XliffParser.php (1)
  • XliffParser (7-76)
src/Result/ValidationResultJsonRenderer.php (2)
src/Result/ValidationResult.php (2)
  • ValidationResult (11-69)
  • getStatistics (65-68)
src/Result/ValidationStatistics.php (6)
  • getExecutionTime (18-21)
  • getExecutionTimeFormatted (23-30)
  • getFilesChecked (32-35)
  • getKeysChecked (37-40)
  • getValidatorsRun (42-45)
  • getParsersCached (47-50)
tests/src/Result/ValidationStatisticsTest.php (1)
src/Result/ValidationStatistics.php (6)
  • getParsersCached (47-50)
  • ValidationStatistics (7-51)
  • getExecutionTime (18-21)
  • getFilesChecked (32-35)
  • getKeysChecked (37-40)
  • getValidatorsRun (42-45)
🔇 Additional comments (26)
src/Result/ValidationResultCliRenderer.php (1)

276-276: LGTM! Clean integration of parser cache statistics.

The implementation correctly follows the established pattern for displaying statistics and properly calls the corresponding getter method.

tests/src/Result/ValidationRunTest.php (1)

257-260: LGTM! Proper mock implementation for the new interface method.

The hardcoded return value is appropriate for testing and aligns with the mock's design pattern.

src/Parser/ParserInterface.php (1)

25-25: All ParserInterface implementers include getFileDirectory()

Verified that every class implementing ParserInterface either declares or inherits the new getFileDirectory(): string method:

  • src/Parser/AbstractParser.php – provides a concrete getFileDirectory() implementation
  • src/Parser/XliffParser.php & src/Parser/YamlParser.php – extend AbstractParser, inheriting the method

No missing implementations found. This breaking change has been applied consistently; you can proceed with coordinating the release.

src/Result/ValidationStatistics.php (2)

14-14: LGTM! Clean addition of parser cache statistics property.

The default value of 0 ensures backward compatibility while the readonly property maintains immutability.


47-50: LGTM! Proper getter implementation.

The method follows the established naming convention and correctly returns the cached parser count.

src/Validator/AbstractValidator.php (1)

8-8: LGTM! Proper import for the new cache functionality.

src/Result/ValidationRun.php (5)

8-8: LGTM - Proper import for ParserCache.

The import is correctly added to support the new parser caching functionality.


52-54: LGTM - Cache statistics collection before clearing.

The cache statistics are correctly collected before clearing the cache, ensuring accurate metrics are captured.


61-62: LGTM - ValidationStatistics constructor updated with parser cache count.

The constructor call is properly updated to include the parsers cached count as the fifth parameter.


65-66: LGTM - Cache clearing after ValidationResult creation.

The cache is appropriately cleared after creating the ValidationResult to prevent memory leaks between validation runs.


103-103: LGTM - Parser instantiation replaced with cache usage.

The change from direct parser instantiation to ParserCache::get() will improve performance by reusing parser instances for the same file/parser class combinations.

src/Result/ValidationResultJsonRenderer.php (2)

29-29: LGTM - Statistics added to JSON output.

The statistics are correctly added to the JSON output structure.


124-142: LGTM - Well-implemented statistics formatting method.

The formatStatisticsForJson method is properly implemented with:

  • Null safety check returning empty array when statistics are unavailable
  • Complete coverage of all statistics fields
  • Appropriate key naming for JSON output
  • Correct return type annotation
tests/src/Result/ValidationResultCliRendererTest.php (4)

380-380: LGTM - Test assertion for default parsers cached value.

The test correctly verifies that the default value of 0 for parsers cached is displayed in verbose mode.


389-390: LGTM - ValidationStatistics constructor updated with parsers cached parameter.

The constructor call is properly updated to include the parsers cached count (7) as the fifth parameter.


403-403: LGTM - Test assertion for explicit parsers cached value.

The test correctly verifies that the explicit value of 7 for parsers cached is displayed in verbose mode.


425-425: LGTM - Test verifies parsers cached is hidden in compact mode.

The test correctly verifies that parsers cached statistics are not shown in compact mode, maintaining the appropriate level of detail for different verbosity levels.

src/Parser/ParserCache.php (1)

7-36: LGTM - Well-implemented parser cache with good design.

The ParserCache class is well-designed with:

  • Clear separation of concerns with static methods
  • Appropriate cache key generation using file path and parser class
  • Proper null safety in the get method
  • Comprehensive cache statistics including both count and keys
  • Clean cache clearing functionality

The implementation will effectively reduce parser instantiation overhead for repeated file/parser combinations.

tests/src/Result/ValidationResultJsonRendererTest.php (4)

11-11: LGTM - Import added for ValidationStatistics.

The import is correctly added to support the new test methods that use ValidationStatistics.


231-253: LGTM - Comprehensive test for statistics rendering.

The test method thoroughly verifies:

  • Correct exit code (0)
  • Presence of statistics key in JSON output
  • Accurate rendering of all statistics fields including execution time, files checked, keys checked, validators run, and parsers cached
  • Proper floating-point comparison using assertEqualsWithDelta

255-269: LGTM - Test for null statistics handling.

The test correctly verifies that when no statistics are provided, the statistics key exists but is empty, ensuring consistent JSON structure.


271-288: LGTM - Test for default parsers cached value.

The test properly verifies that when the parsers cached parameter is not provided to the ValidationStatistics constructor, it defaults to 0 in the JSON output.

tests/src/Parser/ParserCacheTest.php (2)

91-96: Test coverage looks comprehensive.

The test suite covers all the essential functionality of the ParserCache class including instance creation, caching behavior, different file/class handling, cache clearing, and statistics reporting. The tests are well-structured with proper setup/teardown.


74-85: XLIFF test data is well-formed.

The XLIFF XML content is properly structured and contains all necessary elements for testing the XliffParser integration with the cache.

tests/src/Result/ValidationStatisticsTest.php (2)

30-31: Good integration of new property testing.

The existing test method was appropriately updated to verify the default value of the new parsersCached property, ensuring backward compatibility.


33-54: Comprehensive test for explicit parsersCached value.

The new test method thoroughly verifies that when parsersCached is explicitly provided to the constructor, it's properly stored and returned by the getter method.


foreach ($files as $filePath) {
$file = new ($parserClass ?: ParserRegistry::resolveParserClass($filePath))($filePath);
$file = ParserCache::get($filePath, $parserClass ?: ParserRegistry::resolveParserClass($filePath));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential type safety issue with null parser class.

ParserRegistry::resolveParserClass() can return null, but ParserCache::get() expects a string for the parser class parameter. This could cause a runtime error when no parser supports the file type.

Consider adding proper null handling:

-            $file = ParserCache::get($filePath, $parserClass ?: ParserRegistry::resolveParserClass($filePath));
+            $resolvedParserClass = $parserClass ?: ParserRegistry::resolveParserClass($filePath);
+            if (null === $resolvedParserClass) {
+                $this->logger?->warning(sprintf('No parser found for file: %s', $filePath));
+                continue;
+            }
+            $file = ParserCache::get($filePath, $resolvedParserClass);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$file = ParserCache::get($filePath, $parserClass ?: ParserRegistry::resolveParserClass($filePath));
$resolvedParserClass = $parserClass ?: ParserRegistry::resolveParserClass($filePath);
if (null === $resolvedParserClass) {
$this->logger?->warning(sprintf('No parser found for file: %s', $filePath));
continue;
}
$file = ParserCache::get($filePath, $resolvedParserClass);
🤖 Prompt for AI Agents
In src/Validator/AbstractValidator.php at line 46, the call to
ParserCache::get() passes a parser class that may be null due to
ParserRegistry::resolveParserClass() returning null, causing a type safety
issue. To fix this, add a check to ensure the parser class is a valid string
before passing it to ParserCache::get(), and handle the null case appropriately,
such as throwing an exception or providing a default parser class.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Nitpick comments (1)
tests/src/Utility/PathUtilityExtendedTest.php (1)

43-63: Improve filesystem test reliability.

The test creates and removes directories but could be more robust in handling edge cases and cleanup.

Consider using PHPUnit's data providers or a more robust temporary directory approach:

 public function testNormalizeFolderPathWithSubdirectoryOfCwd(): void
 {
-    $currentDir = getcwd();
-    $testDir = $currentDir . '/test_subdir';
-    
-    // Create temporary directory
-    if (!is_dir($testDir)) {
-        mkdir($testDir, 0777, true);
-    }
-    
-    try {
-        $result = PathUtility::normalizeFolderPath($testDir);
-        $this->assertIsString($result);
-        $this->assertSame('test_subdir', $result);
-    } finally {
-        // Clean up
-        if (is_dir($testDir)) {
-            rmdir($testDir);
-        }
-    }
+    $testDir = sys_get_temp_dir() . '/test_subdir_' . uniqid();
+    
+    mkdir($testDir, 0777, true);
+    
+    try {
+        $result = PathUtility::normalizeFolderPath($testDir);
+        $this->assertNotEmpty($result);
+        // Verify it's an absolute path since it's outside cwd
+        $this->assertStringStartsWith('/', $result);
+    } finally {
+        if (is_dir($testDir)) {
+            rmdir($testDir);
+        }
+    }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49c1620 and d24e73c.

📒 Files selected for processing (5)
  • src/Parser/ParserCache.php (1 hunks)
  • src/Parser/ParserRegistry.php (3 hunks)
  • src/Validator/AbstractValidator.php (2 hunks)
  • tests/src/PluginExtendedTest.php (1 hunks)
  • tests/src/Utility/PathUtilityExtendedTest.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/Validator/AbstractValidator.php
  • src/Parser/ParserCache.php
🧰 Additional context used
🪛 PHPStan (2.1.15)
tests/src/Utility/PathUtilityExtendedTest.php

38-38: Call to method PHPUnit\Framework\Assert::assertIsString() with string will always evaluate to true.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.

(method.alreadyNarrowedType)


40-40: Call to function is_string() with string will always evaluate to true.

(function.alreadyNarrowedType)


40-40: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.

(method.alreadyNarrowedType)


55-55: Call to method PHPUnit\Framework\Assert::assertIsString() with string will always evaluate to true.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.

(method.alreadyNarrowedType)


71-71: Call to method PHPUnit\Framework\Assert::assertIsString() with string will always evaluate to true.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.

(method.alreadyNarrowedType)


84-84: Call to method PHPUnit\Framework\Assert::assertIsString() with string will always evaluate to true.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.

(method.alreadyNarrowedType)


86-86: Call to function is_string() with string will always evaluate to true.

(function.alreadyNarrowedType)


86-86: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.

(method.alreadyNarrowedType)

tests/src/PluginExtendedTest.php

30-30: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.

(method.alreadyNarrowedType)


40-40: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.

(method.alreadyNarrowedType)


50-50: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.

(method.alreadyNarrowedType)

🪛 GitHub Actions: CGL
tests/src/Utility/PathUtilityExtendedTest.php

[error] 1-1: EditorConfig: This file has no final new line.


[error] 36-36: EditorConfig: Trailing whitespaces found.


[error] 47-47: EditorConfig: Trailing whitespaces found.


[error] 52-52: EditorConfig: Trailing whitespaces found.


[error] 69-69: EditorConfig: Trailing whitespaces found.

tests/src/PluginExtendedTest.php

[error] 1-1: EditorConfig: This file has no final new line.

🔇 Additional comments (5)
src/Parser/ParserRegistry.php (3)

7-7: LGTM: Clean import addition

The LoggerInterface import is properly added and necessary for the new logging functionality.


25-25: LGTM: Backwards compatible method signature enhancement

The optional logger parameter is well-implemented using nullable type with default null, maintaining backwards compatibility while enabling enhanced logging capabilities.


37-38: LGTM: Appropriate logging implementation with minor consideration

The logging implementation correctly uses the null-safe operator and provides useful diagnostic information when no parser is found.

Consider that the full file path in the log message could potentially expose sensitive directory structures in logs, but this is likely acceptable for debugging purposes and follows common logging practices.

tests/src/PluginExtendedTest.php (1)

1-22: LGTM! Well-structured test class with proper setup.

The class structure, imports, and setup method are well-implemented. The use of PHPUnit attributes and proper dependency injection through mocks shows good testing practices.

tests/src/Utility/PathUtilityExtendedTest.php (1)

95-99: NormalizeFolderPath preserves multiple consecutive slashes for non-existent paths.
Currently, when realpath($path) returns false, normalizeFolderPath only rtrims the trailing slash and does not collapse internal runs of /. As a result,

PathUtility::normalizeFolderPath('/some/path//with///multiple////slashes/')
    // => '/some/path//with///multiple////slashes'

matches the existing test.

• If you’d rather collapse any /{2,} sequences into a single /, add a replacement step (for example,

$normalizedPath = preg_replace('#/{2,}#','/',$normalizedPath);

) before returning and update testNormalizeFolderPathWithMultipleSlashes to expect /some/path/with/multiple/slashes.
• Otherwise, the current behavior and test are consistent and no change is needed.

Comment on lines +33 to +41
public function testDeactivate(): void
{
$composer = $this->createMock(Composer::class);
$io = $this->createMock(IOInterface::class);

// Should not throw any exception
$this->plugin->deactivate($composer, $io);
$this->assertTrue(true); // Method executes without error
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve test assertions to be more meaningful.

Same issue as testActivate - the assertTrue(true) assertion doesn't provide any meaningful verification.

 public function testDeactivate(): void
 {
     $composer = $this->createMock(Composer::class);
     $io = $this->createMock(IOInterface::class);

-    // Should not throw any exception
+    $this->expectNotToPerformAssertions();
     $this->plugin->deactivate($composer, $io);
-    $this->assertTrue(true); // Method executes without error
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function testDeactivate(): void
{
$composer = $this->createMock(Composer::class);
$io = $this->createMock(IOInterface::class);
// Should not throw any exception
$this->plugin->deactivate($composer, $io);
$this->assertTrue(true); // Method executes without error
}
public function testDeactivate(): void
{
$composer = $this->createMock(Composer::class);
$io = $this->createMock(IOInterface::class);
$this->expectNotToPerformAssertions();
$this->plugin->deactivate($composer, $io);
}
🧰 Tools
🪛 PHPStan (2.1.15)

40-40: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.

(method.alreadyNarrowedType)

🤖 Prompt for AI Agents
In tests/src/PluginExtendedTest.php around lines 33 to 41, the testDeactivate
method uses assertTrue(true), which is a meaningless assertion. Replace this
with a more meaningful assertion that verifies expected behavior or state after
calling deactivate, such as checking if certain methods were called on mocks or
if the plugin's state changed as expected.

Comment on lines +23 to +31
public function testActivate(): void
{
$composer = $this->createMock(Composer::class);
$io = $this->createMock(IOInterface::class);

// Should not throw any exception
$this->plugin->activate($composer, $io);
$this->assertTrue(true); // Method executes without error
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve test assertions to be more meaningful.

The assertTrue(true) assertion is always true and doesn't verify any actual behavior. Consider using expectNotToPerformAssertions() if you only want to verify that no exceptions are thrown, or better yet, test actual behavior of the activate method.

 public function testActivate(): void
 {
     $composer = $this->createMock(Composer::class);
     $io = $this->createMock(IOInterface::class);

-    // Should not throw any exception
+    $this->expectNotToPerformAssertions();
     $this->plugin->activate($composer, $io);
-    $this->assertTrue(true); // Method executes without error
 }

Alternatively, if the activate method has observable side effects, consider testing those instead.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function testActivate(): void
{
$composer = $this->createMock(Composer::class);
$io = $this->createMock(IOInterface::class);
// Should not throw any exception
$this->plugin->activate($composer, $io);
$this->assertTrue(true); // Method executes without error
}
public function testActivate(): void
{
$composer = $this->createMock(Composer::class);
$io = $this->createMock(IOInterface::class);
$this->expectNotToPerformAssertions();
$this->plugin->activate($composer, $io);
}
🧰 Tools
🪛 PHPStan (2.1.15)

30-30: Call to method PHPUnit\Framework\Assert::assertTrue() with true will always evaluate to true.

(method.alreadyNarrowedType)

🤖 Prompt for AI Agents
In tests/src/PluginExtendedTest.php around lines 23 to 31, replace the trivial
assertion assertTrue(true) with a more meaningful test. If the goal is only to
verify that no exceptions are thrown, use the PHPUnit method
expectNotToPerformAssertions() to explicitly state this. Otherwise, identify and
assert any observable side effects or state changes caused by the activate
method to make the test more effective.

@konradmichalik konradmichalik merged commit 3b9fad5 into main Jul 10, 2025
22 checks passed
@konradmichalik konradmichalik deleted the parser-cache branch July 28, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant