-
Notifications
You must be signed in to change notification settings - Fork 504
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix fatal error on constant('')
#3013
Conversation
This pull request has been marked as ready for review. |
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'd like to have a stub for Name constructor and all Name subclasses constructors that enforces non-empty-string.
using non-empty-string names everywhere would spread across the whole codebase. I fixed the places which worked without ugly catches and baselined the remaining ones |
tested locally with regarding
also created a upstream PR for PHPParser: nikic/PHP-Parser#993 |
Yes, that's the idea. I'm not looking for a quick fix but a correct fix. Please do not baseline anything. Baseline is not supposed to grow. |
looks like this will be a breaking change and can only be merged for 2.0? |
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.
Please be more careful - in some places where we gain type safety thanks to the new PHPDocs, you instantly throw it away by throwing ShouldNotHappenException
.
src/Type/Php/ArrayFilterFunctionReturnTypeReturnTypeExtension.php
Outdated
Show resolved
Hide resolved
src/Type/Php/ArrayFilterFunctionReturnTypeReturnTypeExtension.php
Outdated
Show resolved
Hide resolved
@@ -127,6 +138,10 @@ public static function fromStubParameter( | |||
|
|||
public static function fromGlobalConstant(ReflectionConstant $constant): self | |||
{ | |||
if ($constant->getNamespaceName() === '') { | |||
throw new ShouldNotHappenException('Namespace cannot be empty.'); |
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.
This could be tackled by a better typehint in BetterReflection.
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.
fix in Roave/BetterReflection#1428
It'd be great if you could finish this one. Thanks. |
I am on it right in this moment :) sorting out dependencies atm |
fix fix fix fix fix Update Type.php Revert "Update Type.php" This reverts commit a239561a19109a244f370a59532891959438f0eb. fix tests fix fix Update phpstan-baseline.neon
I think this one is ready to land. the last remaining error should be fixed with Roave/BetterReflection#1428 |
Thank you! |
|
||
use PhpParser\NodeAbstract; | ||
|
||
class Name extends NodeAbstract |
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 should be able to remove this stub after the next php-parser release because of nikic/PHP-Parser#993
closes phpstan/phpstan#10867
there are only 2 other callers for this very same method in the codebase, and there we already early exit on
''
:e.g.
phpstan-src/src/Type/Php/DefinedConstantTypeSpecifyingExtension.php
Lines 50 to 60 in 5c3a711