Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[ErrorHandler] resolve class constant types when patching return types#58536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedOct 11, 2024
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what will happen here for things likeCookie::SAMESITE_* ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 theresource type.
| $constant =new \ReflectionClassConstant($definingClass,$constantName); | ||
| if (method_exists(\ReflectionClassConstant::class,'getType') &&$constantType =$constant->getType()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we replace themethod_exists check with aPHP_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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| $constant =new \ReflectionClassConstant($definingClass,$constantName); | ||
| if (method_exists(\ReflectionClassConstant::class,'getType') &&$constantType =$constant->getType()) { | ||
| $n =$constantType->getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess the stub is not correct here. I added a test covering this line and$constantType is indeed an instance ofReflectionNamedType.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to60553b9Comparexabbuh commentedOct 11, 2024
@stof test added |
c3f10c3 intosymfony:7.2Uh oh!
There was an error while loading.Please reload this page.
Zausenec commentedNov 29, 2024
With01eb32cdanog\MadelineProto not working Both classes danog\MadelineProto\AbstractAPI and danog\MadelineProto\Namespace\AbstractAPI are exists |
xabbuh commentedNov 30, 2024
@Zausenec please open a new issue and provide a small example application that allows to reproduce it. |
Zausenec commentedNov 30, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
account for types like the ones added in#58493