[ErrorHandler] resolve class constant types when patching return types #58536
Conversation
|
Can you add a test covering this (probably in the existing tests |
| default => $definingClass, | ||
| }; | ||
|
|
||
| if (!\defined($definingClass.'::'.$definingClass)) { |
There was a problem hiding this comment.
what will happen here for things like Cookie::SAMESITE_* ?
There was a problem hiding this comment.
We ignore them for now. The changes from this PR require that you use the full constant name. We could ship support for wildcards in a follow-up PR (I would like to ship this one as soon as possible to make the job green again and supporting wildcards requires a bit more work).
There was a problem hiding this comment.
I'm fine with ignoring them (we were already ignoring those cases before, see the comment below). But I'm wondering whether this will be ignored cleaning here.
There was a problem hiding this comment.
Yes, returning here (which means not reaching the line where the $returnTypes property is written for the current method means that no patching happens afterwards. The same is done for example further down for the resource type.
|
|
||
| $constant = new \ReflectionClassConstant($definingClass, $constantName); | ||
|
|
||
| if (method_exists(\ReflectionClassConstant::class, 'getType') && $constantType = $constant->getType()) { |
There was a problem hiding this comment.
Can we replace the method_exists check with a PHP_VERSION_ID check ? This has 2 benefits:
- it will make it clearer when we can simplify that check thanks to our min requirement
- it will be resolved at compile-time
| $constant = new \ReflectionClassConstant($definingClass, $constantName); | ||
|
|
||
| if (method_exists(\ReflectionClassConstant::class, 'getType') && $constantType = $constant->getType()) { | ||
| $n = $constantType->getName(); |
There was a problem hiding this comment.
is the constant type guaranteed to be a ReflectionNamedType in PHP ? https://github.com/php/php-src/blob/9345582471d5dfa8b783b1e666d8b76620f542f9/ext/reflection/php_reflection.stub.php#L626C32-L626C47 seems to answer "no"
There was a problem hiding this comment.
I guess the stub is not correct here. I added a test covering this line and $constantType is indeed an instance of ReflectionNamedType.
I stand corrected here. A constant can indeed have union or intersection types (see the examples in the RFC: https://wiki.php.net/rfc/typed_class_constants).
There was a problem hiding this comment.
well, if the type you use is a named type, it will. But do constant types allows using union or intersection types ? Those are not named types.
There was a problem hiding this comment.
Yes, they can be union or intersection types (see my edit above). I've opted to ignore them for now in this PR. We can consider to add support for them in the future (similar to the support for wildcards). But I do not see an urgent need for that right now (for wildcards that's a bit different as we make use of them in Symfony).
3c91c04 to
60553b9
Compare
|
@stof test added |
|
With 01eb32c danog\MadelineProto not working Both classes danog\MadelineProto\AbstractAPI and danog\MadelineProto\Namespace\AbstractAPI are exists |
|
@Zausenec please open a new issue and provide a small example application that allows to reproduce it. |
account for types like the ones added in #58493