[PropertyInfo] strip only leading \ when unknown docType
#45720
Conversation
| @@ -164,6 +164,6 @@ private function getPhpTypeAndClass(string $docType): array | |||
| return ['object', $docType]; | |||
| } | |||
|
|
|||
| return ['object', substr($docType, 1)]; | |||
| return ['object', ltrim($docType, '\\')]; // strip the namespace's `\`-prefix | |||
There was a problem hiding this comment.
With this "fix" pseudo types will be considered as an object. That's wrong.
object(Symfony\Component\PropertyInfo\Type)#15949 (6) {
["builtinType":"Symfony\Component\PropertyInfo\Type":private]=>
string(6) "object"
["nullable":"Symfony\Component\PropertyInfo\Type":private]=>
bool(false)
["class":"Symfony\Component\PropertyInfo\Type":private]=>
string(14) "numeric-string"
["collection":"Symfony\Component\PropertyInfo\Type":private]=>
bool(false)
["collectionKeyType":"Symfony\Component\PropertyInfo\Type":private]=>
array(0) {
}
["collectionValueType":"Symfony\Component\PropertyInfo\Type":private]=>
array(0) {
}
}There was a problem hiding this comment.
pseudo types will be considered as an object
My fix doesn't change that. Before the fix it is an object too (but the $class property value has the first character trimmed, and this bug is the subject of my fix):
class Symfony\Component\PropertyInfo\Type#1168 (6) {
private $builtinType =>
string(6) "object"
private $nullable =>
bool(false)
private $class =>
string(13) "umeric-string"
private $collection =>
bool(false)
private $collectionKeyType =>
NULL
private $collectionValueType =>
NULL
}Although it may be NULL in some cases. I think it depends on phpdocumentor version, but I'm not sure yet. Can you confirm?
By the way, if you expected numeric-string pseudo-type to be mapped to string built-in type, it will be done so in Symfony 6.1 thanks to #44451
There was a problem hiding this comment.
Yes, currently it will be mapped to object (and the first character got removed).
But think about your fix. Is numeric-string really an object? No.
And in some software there will be done additional checks/procedures when it's an object. So this commit isn't really an fix.
There was a problem hiding this comment.
I'm not trying here to fix the issue with psuedo-types being treated as object. It still is a big issue and I believe this behavior cannot be changed in older branches as this will be a BC break. It is undocumented feature - the documentation does not promise that the component handles pseudo-types and before v6.1 it only handles a few of them. In my opinion we should implement it in major version and that's why I created #44455. But it's a hard problem to solve.
At the moment the least I can do without causing BC break is to merely fix the very issue with the first character being removed. If we fix it, I can work around the problem somehow (e.g. by checking if class_exists or by comparing it to predefined pseudo-types strings) in my project, what I can't do without the fix because the original phpdoc is in fact lost due to this bug.
Is numeric-string really an object? No.
Due to string being a reserved keyword in PHP and - which is not allowed in a class name it never is. But consider this example:
class SomeClass
{
/**
* @var scalar
*/
public $scalarProperty;
}
class scalar {}scalar is a pseudo-type handled by PHPStan. But if the scalar class is defined, it may as well be an object. Before the fix, extractor gives me an object with calar className and I expect scalar - that my fix resolves.
|
The test failure looks realted: |
|
Thank you @EmilMassey. |
|
I'm using NelmioApiDocBundle, and the following code started breaking with : public function supports(Model $model): bool
{
return Enum::class === get_parent_class((string) $model->getType()->getClassName());
}The problem being that now sometimes |
|
@gnutix Thanks for noticing the bug. Is the code snippet from the bundle or it is a fragment of your project? If it’s from the bundle, could you please link the source? Maybe it’s a good idea to create a new issue so it can be seen by more people as this PR is already closed? |
|
@EmilMassey I've finally had the time to look at it again, and fixed it. It was a bad typing on our part in a constructor : /** @var array<?ExtraHoursReportCell> */
public array $weeks,The |
|
I'm glad you've managed to solve the problem, and thanks for the response :) |
Fixes issue in
PhpDocExtractorwhen dealing with some pseudo-types (non-empty-string,positive-int, etc.):The bug exists in 4.4, 5.4 and 6.0. It may exist in 6.1 as the invalid line is still there, but due to changes made in #44451, above snippet will no longer produce the bug. I was not able to reproduce the error in 6.1, but I suspect there may exist some pseudo-type that is still affected by the bug.
I'm not sure if the test I wrote is OK, but it is the only way I could think of to validate my fix. In my opinion (see #44455), unknown pseudo-type should not be mapped to an object, so after the fix handling of pseudo-types is still broken but at least the results are consistent between
PhpStanExtractorandPhpDocExtractor. But I agree with @derrabus (#44455 (comment)) that until there is no way to detect if we are dealing with a pseudo-type, assuming it is a reference to an object is the sanest thing to do.