Conversation
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/33615c68-b66a-4778-b5f6-2a0dfe5b97bd Co-authored-by: voku <264695+voku@users.noreply.github.com>
@param phpDocRaw preservation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new test case, testBrokenParamPhpDocRawIsPreserved, to ensure that malformed PhpDoc parameters are correctly handled by preserving the raw string and returning a null extended type. Feedback indicates that this new test is redundant because the same assertions are already covered in testSimpleOneClassWithTrait, suggesting that the duplicate logic should be consolidated.
| public function testBrokenParamPhpDocRawIsPreserved(): void | ||
| { | ||
| if (PHP_VERSION_ID < 80000) { | ||
| static::markTestSkipped('only for PHP >= 8.0'); | ||
| } | ||
|
|
||
| $phpCode = PhpCodeParser::getPhpFiles(__DIR__ . '/Dummy8.php'); | ||
| $phpClasses = $phpCode->getClasses(); | ||
|
|
||
| static::assertSame( | ||
| 'array{stdClass: \stdClass, numbers: int|float $lall <foo/>', | ||
| $phpClasses[Dummy8::class]->methods['foo_broken']->parameters['lall']->phpDocRaw | ||
| ); | ||
|
|
||
| static::assertNull($phpClasses[Dummy8::class]->methods['foo_broken']->parameters['lall']->typeFromPhpDocExtended); | ||
| } |
There was a problem hiding this comment.
The test method testBrokenParamPhpDocRawIsPreserved is redundant. The exact same assertions for Dummy8::foo_broken (checking both phpDocRaw and typeFromPhpDocExtended) are already implemented in testSimpleOneClassWithTrait on lines 205-208 and 217.
Duplicate test logic increases maintenance overhead without providing additional coverage. If the goal is to have a focused test for this regression, consider moving the assertions from testSimpleOneClassWithTrait into this new method instead of duplicating them.
There was a problem hiding this comment.
Pull request overview
Adds a focused regression test to ensure malformed @param phpDoc text is preserved verbatim in phpDocRaw while remaining unparseable as an extended type.
Changes:
- Added
testBrokenParamPhpDocRawIsPreserved()targetingDummy8::foo_broken()’s malformed@paramhandling. - Asserted the raw malformed
@paramstring is preserved exactly inparameters['lall']->phpDocRaw. - Asserted the same malformed value still does not produce
typeFromPhpDocExtended.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function testBrokenParamPhpDocRawIsPreserved(): void | ||
| { | ||
| if (PHP_VERSION_ID < 80000) { | ||
| static::markTestSkipped('only for PHP >= 8.0'); | ||
| } | ||
|
|
||
| $phpCode = PhpCodeParser::getPhpFiles(__DIR__ . '/Dummy8.php'); | ||
| $phpClasses = $phpCode->getClasses(); | ||
|
|
||
| static::assertSame( | ||
| 'array{stdClass: \stdClass, numbers: int|float $lall <foo/>', | ||
| $phpClasses[Dummy8::class]->methods['foo_broken']->parameters['lall']->phpDocRaw | ||
| ); | ||
|
|
||
| static::assertNull($phpClasses[Dummy8::class]->methods['foo_broken']->parameters['lall']->typeFromPhpDocExtended); |
There was a problem hiding this comment.
This test duplicates assertions already present in testSimpleOneClassWithTrait() for Dummy8::foo_broken() (see tests/ParserTest.php around the existing phpDocRaw / typeFromPhpDocExtended checks). To reduce duplication/maintenance, consider either (a) moving those assertions out of testSimpleOneClassWithTrait() into this focused test and removing the originals, or (b) extracting a small helper that returns the parsed Dummy8 class so both tests don’t need to re-parse Dummy8.php.
| public function testBrokenParamPhpDocRawIsPreserved(): void | |
| { | |
| if (PHP_VERSION_ID < 80000) { | |
| static::markTestSkipped('only for PHP >= 8.0'); | |
| } | |
| $phpCode = PhpCodeParser::getPhpFiles(__DIR__ . '/Dummy8.php'); | |
| $phpClasses = $phpCode->getClasses(); | |
| static::assertSame( | |
| 'array{stdClass: \stdClass, numbers: int|float $lall <foo/>', | |
| $phpClasses[Dummy8::class]->methods['foo_broken']->parameters['lall']->phpDocRaw | |
| ); | |
| static::assertNull($phpClasses[Dummy8::class]->methods['foo_broken']->parameters['lall']->typeFromPhpDocExtended); | |
| private function getParsedDummy8Class() | |
| { | |
| $phpCode = PhpCodeParser::getPhpFiles(__DIR__ . '/Dummy8.php'); | |
| $phpClasses = $phpCode->getClasses(); | |
| return $phpClasses[Dummy8::class]; | |
| } | |
| public function testBrokenParamPhpDocRawIsPreserved(): void | |
| { | |
| if (PHP_VERSION_ID < 80000) { | |
| static::markTestSkipped('only for PHP >= 8.0'); | |
| } | |
| $phpClass = $this->getParsedDummy8Class(); | |
| static::assertSame( | |
| 'array{stdClass: \stdClass, numbers: int|float $lall <foo/>', | |
| $phpClass->methods['foo_broken']->parameters['lall']->phpDocRaw | |
| ); | |
| static::assertNull($phpClass->methods['foo_broken']->parameters['lall']->typeFromPhpDocExtended); |
|
@copilot expand the new test by getting all the information that we can from our libary, focus on the types and run a blind spot analysis for yourself based on the provided results afterwards |
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/b9164c9a-90b6-4a4b-a219-02826a23247b Co-authored-by: voku <264695+voku@users.noreply.github.com>
Expanded in f4f4072. The focused test now covers the broken param/return type projections ( |
@param phpDocRaw preservation@param type handling and phpDocRaw preservation
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/80d2c8c5-8ef5-43ea-a472-532d966705a8 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/80d2c8c5-8ef5-43ea-a472-532d966705a8 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/80d2c8c5-8ef5-43ea-a472-532d966705a8 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/80d2c8c5-8ef5-43ea-a472-532d966705a8 Co-authored-by: voku <264695+voku@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request introduces support for PHP 8.4 features, specifically property hooks and asymmetric visibility, across the parser models and reflection logic. It also implements a recovery mechanism for broken PHPDoc types by attempting to parse partial strings when a full parse fails. Key changes include updates to PHPClass, PHPProperty, and PHPParameter to handle the new syntax, as well as the addition of several test fixtures. Review feedback identifies a potential runtime error in PHPParameter due to a missing method call and suggests refactoring redundant autoloading logic in PHPClass to use a shared base class helper.
| $this->typeFromPhpDocExtended = $recoveredType; | ||
| } | ||
|
|
||
| $this->addParseError($e); |
There was a problem hiding this comment.
The method addParseError() does not appear to be defined in PHPParameter or its base class BasePHPElement. This will likely result in a fatal error at runtime. Based on the implementation in PHPFunction.php, you should manually add the error to the parseError array.
$tmpErrorMessage = $parameterName . ':' . ($this->line ?? '?') . ' | ' . $e->getMessage();
$this->parseError[\md5($tmpErrorMessage)] = $tmpErrorMessage;| $canAutoload = (\PHP_VERSION_ID >= 80200 || !self::nodeUsesPHP82PlusSyntax($node)) | ||
| && (\PHP_VERSION_ID >= 80300 || !self::nodeUsesPHP83PlusSyntax($node)); | ||
| && (\PHP_VERSION_ID >= 80300 || !self::nodeUsesPHP83PlusSyntax($node)) | ||
| && (\PHP_VERSION_ID >= 80400 || !self::containsPHP84PlusSyntax($node)); |
There was a problem hiding this comment.
This logic is redundant as it is now encapsulated in the canAutoloadFromPhpNode() method in the base class BasePHPClass. Reusing the shared helper improves maintainability and aligns with the PR's objective of removing redundant class-specific logic.
$canAutoload = self::canAutoloadFromPhpNode($node);Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/495a8a5d-b52e-4989-a07f-42862007b8b9 Co-authored-by: voku <264695+voku@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static function recoverBrokenPhpdocType(string $input): ?string | ||
| { | ||
| $parts = []; | ||
| foreach (self::modernPhpdocTokens($input)->getTokens() as $token) { | ||
| if (!empty($token[0])) { | ||
| $parts[] = $token[0]; | ||
| } | ||
| } | ||
|
|
||
| for ($i = \count($parts); $i > 0; --$i) { | ||
| $candidate = \trim(\implode('', \array_slice($parts, 0, $i))); | ||
| if ($candidate === '') { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| return self::modernPhpdoc($candidate); | ||
| } catch (\PHPStan\PhpDocParser\Parser\ParserException $e) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
recoverBrokenPhpdocType() retries parsing by building progressively shorter candidates and calling modernPhpdoc() each time (which re-tokenizes and re-parses). In the worst case this is O(n²) and can become expensive when many invalid phpdocs are encountered in a large codebase. Consider reusing the initial tokenization (avoid re-lexing), and/or adding a small upper bound on retry attempts (e.g., stop after N failures or when the candidate length drops below a threshold).
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/3fa9fd4e-ddc1-4da8-9b63-a96858874230 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/3fa9fd4e-ddc1-4da8-9b63-a96858874230 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Applied in 9433ca1. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for PHP 8.4 features, including property hooks and asymmetric visibility, while also improving the handling of broken PHPDoc types through a new recovery mechanism. The changes include updates to the AST model to capture these new property attributes and a refactoring of how promoted properties are merged. Feedback is provided regarding the efficiency of locating the constructor in PHPClass.php and a potential issue where promoted property visibility might not correctly override default values from PHPDoc tags.
| foreach ($node->getMethods() as $method) { | ||
| if ($method->name->name !== '__construct') { | ||
| continue; | ||
| } | ||
|
|
||
| foreach ($node->stmts as $stmt) { | ||
| if ($stmt instanceof \PhpParser\Node\Stmt\ClassMethod) { | ||
| if (self::containsPHP82PlusType($stmt->returnType)) { | ||
| return true; | ||
| foreach ($method->params as $parameter) { | ||
| if (!self::isPromotedParameter($parameter)) { | ||
| continue; | ||
| } | ||
| foreach ($stmt->params as $param) { | ||
| if (self::containsPHP82PlusType($param->type)) { | ||
| return true; | ||
| } | ||
|
|
||
| $parameterVar = $parameter->var; | ||
| if ( | ||
| !($parameterVar instanceof \PhpParser\Node\Expr\Variable) | ||
| || !\is_string($parameterVar->name) | ||
| ) { | ||
| continue; | ||
| } | ||
| } elseif ($stmt instanceof \PhpParser\Node\Stmt\Property) { | ||
| if (self::containsPHP82PlusType($stmt->type)) { | ||
| return true; | ||
|
|
||
| $promotedProperty = (new PHPProperty($this->parserContainer)) | ||
| ->readObjectFromPromotedParam($parameter, $this->name); | ||
|
|
||
| $propertyName = $parameterVar->name; | ||
| $existingProperty = $this->properties[$propertyName] ?? null; | ||
| if ($existingProperty !== null) { | ||
| $this->mergePromotedPropertyData($existingProperty, $promotedProperty, $parameter); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| $this->properties[$propertyName] = $promotedProperty; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Instead of iterating through all methods to find the constructor, you can use the getMethod() helper provided by the Class_ node. This is more efficient and makes the code cleaner.
$method = $node->getMethod('__construct');
if ($method === null) {
return;
}
foreach ($method->params as $parameter) {
if (!self::isPromotedParameter($parameter)) {
continue;
}
$parameterVar = $parameter->var;
if (
!($parameterVar instanceof \PhpParser\Node\Expr\Variable)
|| !\is_string($parameterVar->name)
) {
continue;
}
$promotedProperty = (new PHPProperty($this->parserContainer))
->readObjectFromPromotedParam($parameter, $this->name);
$propertyName = $parameterVar->name;
$existingProperty = $this->properties[$propertyName] ?? null;
if ($existingProperty !== null) {
$this->mergePromotedPropertyData($existingProperty, $promotedProperty, $parameter);
continue;
}
$this->properties[$propertyName] = $promotedProperty;
}| if ($existingProperty->access === '' && $promotedProperty->access !== '') { | ||
| $existingProperty->access = $promotedProperty->access; | ||
| } |
There was a problem hiding this comment.
The condition existingProperty->access === '' is too restrictive. When a property is extracted from a PHPDoc @property tag (in readPhpDocProperties), its access is defaulted to 'public'. If that same property is later found as a promoted constructor parameter with a different visibility (e.g., private), the promoted visibility should take precedence. Removing the check for an empty string ensures the actual visibility modifier from the code is used.
if ($promotedProperty->access !== '') {
$existingProperty->access = $promotedProperty->access;
}Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/3f1fe6dd-2d57-407d-868c-88ddcea94b17 Co-authored-by: voku <264695+voku@users.noreply.github.com>
regardless of tag order Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/65b3abc2-0923-4d1d-b8b9-eac5d1c7fb95 Co-authored-by: voku <264695+voku@users.noreply.github.com>
…rruption and modernPhpdoc DNF-type paren mangling Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/0b99f73d-a186-4d7c-86b0-69b4b663c5a4 Co-authored-by: voku <264695+voku@users.noreply.github.com>
|
splitTypeAndVariable()usesstr_replace($variableName, ...)which corrupts array shape types when a key name matches the parameter variable name (e.g.,array{userId: int} $userId→array{: int} $)parseModernPhpdocTokens()usestrim(str, ')(')which aggressively strips parentheses from DNF types (e.g.,(Foo&Bar)|null→Foo & Bar)|null)Utils::splitTypeAndVariable()— replace$variableNamewith'$' . $variableNamein str_replaceUtils::parseModernPhpdocTokens()— strip exactly one pair of balanced outer parens instead of usingtrimtestArrayShapeKeyMatchingVariableNameIsNotCorruptedtestDnfPhpDocTypeIsNotCorrupted