-
Notifications
You must be signed in to change notification settings - Fork 510
Fix 2914 (rebased): Merge PHPDoc block tags when inheriting #196
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 2914 (rebased): Merge PHPDoc block tags when inheriting #196
Conversation
46eb7de
to
1b6acac
Compare
README.md
Outdated
|
||
The executable is bin/phpstan | ||
Rut it as | ||
bin/phpstan analyze -c your-config.neon -l 8 path-to-analyze |
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.
Should use code fences (like other similar commands in this file)
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.
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.
The development version should be run as vendor/bin/phing phpstan
instead.
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.
vendor/bin/phing phpstan
is to run phpstan
on itself. I mean the command to analyse custom code using dev phpstan as one modifies phpstan.
README.md
Outdated
Run: | ||
```bash | ||
export PHPSTAN_ALLOW_XDEBUG=1 | ||
vendor/bin/phpunit --bootstrap=tests/bootstrap.php --stop-on-failure tests |
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.
does PHPSTAN_ALLOW_XDEBUG=1 vendor/bin/phpunit --bootstrap=tests/bootstrap.php --stop-on-failure tests
work, or it needs to be exported?
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 should work as well, but I find export
easier to read.
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.
Not sure what you're referring to, this variable isn't anywhere in the codebase: https://github.com/phpstan/phpstan-src/search?q=PHPSTAN_ALLOW_XDEBUG&unscoped_q=PHPSTAN_ALLOW_XDEBUG
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.
but export
sets variable for whole terminal session, if i prepend it to command, it should be set only for that process
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.
Run this with xdebug enabled:
vendor/bin/phpunit --bootstrap=tests/bootstrap.php --stop-on-failure --filter=testConfigurationAutoDiscovery tests/PHPStan/Command/AnalyseCommandTest.php
The following will happen:
--xdebug
option is not passed tophpstan
by the test.- CommandHelper checks
!$allowXdebug
and calls XdebugHandler::check(). - XdebugHandler checks getenv($this->envAllowXdebug) and finds that it is not set.
- XdebugHandler then restarts the command using the only command it can find:
'/usr/bin/php7.3' '-n' '-c' '/tmp/rIqFYd' '/home/alexey/work/phpstan-src/vendor/phpunit/phpunit/phpunit' '--bootstrap=tests/bootstrap.php' '--stop-on-failure' '--filter=testConfigurationAutoDiscovery' 'tests/PHPStan/Command/AnalyseCommandTest.php' '--ansi'
- PHPUnit dies as it cannot recognize the
--ansi
option.
The stack of this restart is
/home/alexey/work/phpstan-src/vendor/composer/xdebug-handler/src/XdebugHandler.php:136
/home/alexey/work/phpstan-src/src/Command/CommandHelper.php:51
/home/alexey/work/phpstan-src/src/Command/AnalyseCommand.php:126
/home/alexey/work/phpstan-src/vendor/symfony/console/Command/Command.php:255
/home/alexey/work/phpstan-src/vendor/symfony/console/Tester/CommandTester.php:76
/home/alexey/work/phpstan-src/tests/PHPStan/Command/AnalyseCommandTest.php:84
/home/alexey/work/phpstan-src/tests/PHPStan/Command/AnalyseCommandTest.php:25
/home/alexey/work/phpstan-src/vendor/phpunit/phpunit/src/Framework/TestCase.php:1154
The way to prevent this is to set the environment variable with a name constructed in XdebugHandler::__construct
as
$this->envAllowXdebug = self::$name.self::SUFFIX_ALLOW;
which results in the name of PHPSTAN_ALLOW_XDEBUG
.
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 you test this has the same effect? Thanks! 6fa7b67
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, it worked. You may want to add a comment there since as you said PHPSTAN_ALLOW_XDEBUG
is not in the codebase.
8bcb460
to
127bb52
Compare
Added the README update to master separately: e96473d and forcepushed this branch. I'm gonna start review, it's possible I'll divide it more again :) |
048ccf9
to
ea57a4f
Compare
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 like this a lot! BTW have you considered what should be done about @template T
and for example @param T
tags? They can't be simply merged, the scope of the types (TemplateTypeScope) needs to change to reflect the child class.
I'm gonna merge this, please send a new PR with the fixed test. Thanks <3
|
||
public function method(): void | ||
{ | ||
// assertType('InheritDocMergingVar\B', $this->property); |
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 should pass but fails, that's why it's commented for now.
Also feel free to check out the commits I added. The main reason why I changed the tests is that checking out types inside the method (through PhpFunctionFromParserNodeReflection) and checking the types outside the method (getting the return type from Reflection) are two different code paths - see that NodeScopeResolver::getPhpDocs is called in itself (for PhpFunctionFromParserNodeReflection), and in PhpClassReflectionExtension (for outsiders). |
This is "a good thing" (TM) - thanks.
and the implementation has no return statement at all. Probably in that case the interface should just have:
And that keeps phpstan happy that the implementation has no return statement at all. Otherwise at the end of the method implementation I have to add:
which seems a crappy thing to do. |
This is expected - void and null and interchangeable as values in PHP (void method returns null) but not types, so PHPStan does the right thing here - implementation should follow interface. |
@alexeyinkin @ondrejmirtes I think there is something wrong with throws tag. When writing
Since B::foo has no
This is a big issue for https://packagist.org/packages/pepakriz/phpstan-exception-rules users. |
This is expected. See phpstan/phpstan#3350 |
A rebased copy of #193