fix: add support for Lexer v1 (allowed by ORM < v2.15) #300
Conversation
WalkthroughThe changes integrate Doctrine Lexer support within both the CI workflow and the core code. The CI job configuration now includes matrix entries for multiple Doctrine Lexer versions alongside expanded Doctrine ORM versions, with additional inclusion and exclusion rules. In the application code, the direct access to the lexer’s lookahead type in the AST function has been refactored to use a new utility method from a newly added Changes
Sequence Diagram(s)sequenceDiagram
participant CI as "CI Workflow"
participant Matrix as "Matrix Config"
participant Installer as "Dependency Installer"
participant Tester as "Test Runner"
CI->>Matrix: Fetch PHP, Doctrine ORM & Lexer versions
Matrix-->>CI: Provide configuration
CI->>Installer: Install Doctrine ORM dependency
CI->>Installer: Install Doctrine Lexer dependency
Installer-->>CI: Dependencies installed
CI->>Tester: Execute tests
sequenceDiagram
participant BVF as "BaseVariadicFunction"
participant DL as "DoctrineLexer"
participant L as "Lexer"
BVF->>DL: getLookaheadType(Lexer)
DL->>DL: Check isPre200(Lexer)
alt Lexer is pre-200
DL->>L: Retrieve lookahead['type']
else
DL->>L: Retrieve lookahead?->type
end
DL-->>BVF: Return determined type
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/MartinGeorgiev/Utils/DoctrineLexer.php (1)
1-19: Well-designed version detection method.The
isPre200method provides a clean approach to detecting Lexer versions based on the structure of thelookaheadproperty.Consider adding a brief doc comment explaining the version detection strategy:
+ /** + * Checks if the Lexer is prior to version 2.0.0 + * + * In Lexer versions prior to 2.0.0, the lookahead property is an array, + * while in 2.0.0+ it's an object. + */ public static function isPre200(Lexer $lexer): bool { // @phpstan-ignore-next-line return \is_array($lexer->lookahead); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(2 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php(2 hunks)src/MartinGeorgiev/Utils/DoctrineLexer.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (3)
src/MartinGeorgiev/Utils/DoctrineLexer.php (2)
DoctrineLexer(12-33)getLookaheadType(23-32)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/ParserException.php (3)
ParserException(7-18)missingLookaheadType(9-12)withThrowable(14-17)src/MartinGeorgiev/Utils/DoctrineOrm.php (2)
DoctrineOrm(12-18)isPre219(14-17)
🪛 GitHub Check: PHP + Doctrine ORM 2.14 + Doctrine Lexer 1.2
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php
[failure] 46-46:
Parameter #1 $token of method Doctrine\ORM\Query\Parser::match() expects 1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19|100|101|102|200|201|202|203|204|205|206|207|208|209|210|211|212|213|214|215|216|217|218|219|220|221|222|223|224|225|226|227|228|229|230|231|232|233|234|235|236|237|238|239|240|241|242|243|244|245|246|247|248|249|250|251|252|253|254|255|256, mixed given.
🔇 Additional comments (10)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (4)
14-14: Good addition of the DoctrineLexer utility class import.The import of the new DoctrineLexer utility class helps abstract version-specific lexer operations.
34-36: Improved version compatibility with abstracted lexer access.The refactoring to use
DoctrineLexer::getLookaheadType($lexer)instead of direct property access provides better compatibility with different Doctrine Lexer versions.
51-51: Good reuse of the utility method for lookahead type.Consistent usage of
DoctrineLexer::getLookaheadType($lexer)throughout the function maintains code clarity.
44-45:❓ Verification inconclusive
Ensure consistent type safety across lexer versions.
The conditional expressions look good, but the static analysis tool flags a type issue on line 46.
🏁 Script executed:
#!/bin/bash # Check the actual return types of these constants to understand the type mismatch echo "Checking Lexer constant types:" ast-grep --pattern 'class Lexer { $$$ const T_COMMA = $val; $$$ }' | grep -A 3 "T_COMMA" echo "\nChecking TokenType constant types:" ast-grep --pattern 'class TokenType { $$$ const T_COMMA = $val; $$$ }' | grep -A 3 "T_COMMA"Length of output: 335
Manual Verification Required: Validate Lexer Constant Types
The conditional expressions seem appropriate; however, the static analysis tool’s type complaint on line 46 likely stems from a mismatch in the type declarations between
Lexer::T_COMMAandTokenType::T_COMMA. Currently, our automated search did not produce results confirming the expected type definitions for these constants. You should manually verify that both theLexerandTokenTypeclasses declare theT_COMMA(and similarly,T_CLOSE_PARENTHESIS) constants with consistent types to ensure type safety across lexer versions.
- Action Items:
- Manually inspect the
Lexerclass (and any related files) for the declaration ofT_COMMAand confirm its type.- Do the same for the
TokenTypeclass and verify that itsT_COMMAmatches in type.- Review the static analysis configuration to ensure that it correctly interprets the constants' types.
src/MartinGeorgiev/Utils/DoctrineLexer.php (1)
20-32: Good implementation of version-aware lookahead type access.The method properly handles both pre-2.0.0 and 2.0.0+ Lexer versions, providing a unified interface for accessing the lookahead type.
.github/workflows/ci.yml (5)
14-14: Good update to job name reflecting added Lexer dependency.The job name now properly includes the Doctrine Lexer version, providing better clarity in CI logs.
21-22: Comprehensive matrix strategy for testing different versions.The matrix now includes both Doctrine ORM and Lexer versions, ensuring compatibility across different version combinations.
24-32: Well-configured test inclusion and exclusion rules.The inclusion of ORM 2.14 + Lexer 1.2 directly tests the primary use case mentioned in the PR title. The exclusion rules prevent testing incompatible combinations, optimizing CI resources.
57-68: Good implementation of Doctrine Lexer installation step.The conditional installation ensures the correct Lexer version is used for each test combination, with proper fallback to latest when needed.
73-74: Good addition of Doctrine ORM 2.18 installation step.This addition completes the test matrix for all specified ORM versions.
…dependency conflicts when installing ORM
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/MartinGeorgiev/Utils/DoctrineLexer.php (2)
1-59: Well-structured utility class for handling Lexer version differences.The new DoctrineLexer class effectively abstracts the differences between Lexer v1 and v2, centralizing this logic to avoid duplication across the codebase. The implementation correctly handles the structural differences between versions.
Consider adding more specific return type hints and improving the error handling in the edge cases:
- /** - * @return mixed|null - */ + /** + * @return int|string|null The lookahead type, which could be an int (for token constants) or string + */Also, you might want to consider adding a unit test specifically for this class to ensure it works correctly with both Lexer versions.
47-49: Consider more explicit null check for token.The null check approach is good, but you might make it slightly more explicit for better readability.
- if ($lexer->token === null) { - return null; - } + // Early return if token is null to avoid undefined array key access + if ($lexer->token === null) { + return null; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ci/rector/config.php(2 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Cast.php(2 hunks)src/MartinGeorgiev/Utils/DoctrineLexer.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
🔇 Additional comments (3)
ci/rector/config.php (1)
5-5: Skip rule for new DoctrineLexer implementation looks good.Adding the
FlipTypeControlToUseExclusiveTypeRectorto the skip list for the new DoctrineLexer file is appropriate. This prevents the automated refactoring rule from potentially breaking the version-checking logic in the new class.Also applies to: 44-46
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Cast.php (1)
13-13: Good refactoring to use the new DoctrineLexer utility class.The code has been simplified by delegating token access to the new DoctrineLexer utility class. This centralizes the logic for handling different Lexer versions and makes the code more maintainable.
Also applies to: 43-45
src/MartinGeorgiev/Utils/DoctrineLexer.php (1)
22-23:✅ Verification successful
Verify compatibility with all supported Lexer versions.
The implementation assumes that pre-2.0.0 versions use arrays for
lookaheadwhile 2.0.0+ use objects. Make sure this is verified against all supported versions.
🏁 Script executed:
#!/bin/bash # Check if the codebase has tests covering both Lexer versions echo "Checking for tests with different Lexer versions..." rg -l "Lexer.*1\.0|1\.1|1\.2" --type php rg -l "Lexer.*2\.0|2\.1|2\.2" --type php # Check if there are any CI jobs testing both versions echo "Checking for CI jobs with different Lexer versions..." rg -l "doctrine/lexer.*1\." .github/workflows/ rg -l "doctrine/lexer.*2\." .github/workflows/Length of output: 1021
Compatibility Across Supported Lexer Versions Verified
The CI configuration and test coverage confirm that both pre-2.0.0 and 2.0.0+ Lexer behaviors are correctly handled. Specifically:
- Tests in
tests/MartinGeorgiev/Utils/DataStructureTest.phpandsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Ilike.phpcover the pre-2.0.0 behavior.- Additional tests in
src/MartinGeorgiev/Utils/DoctrineLexer.phpand related function tests validate the 2.0.0+ behavior.- The CI workflow file
.github/workflows/ci.ymlincludes checks for both versions.No further adjustments are required.
Summary by CodeRabbit