-
Notifications
You must be signed in to change notification settings - Fork 481
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
emit warnings when xdebug is not used according to cli-flags #1878
Conversation
92e9daf
to
49add99
Compare
(sorry for the typo in the PR title) |
This pull request has been marked as ready for review. |
src/Command/CommandHelper.php
Outdated
$stdOutput = new SymfonyOutput($output, new SymfonyStyle(new ErrorsConsoleStyle($input, $output))); | ||
|
||
/** @var Output $errorOutput */ | ||
$errorOutput = (static function () use ($input, $output): Output { | ||
$symfonyErrorOutput = $output instanceof ConsoleOutputInterface ? $output->getErrorOutput() : $output; | ||
return new SymfonyOutput($symfonyErrorOutput, new SymfonyStyle(new ErrorsConsoleStyle($input, $symfonyErrorOutput))); | ||
})(); | ||
|
||
if ($allowXdebug && !XdebugHandler::isXdebugActive()) { | ||
$errorOutput->writeLineFormatted(sprintf('Note: You are running with "--xdebug" enabled, but the xdebug php-extension is not active.' . "\n" . ' The process will not halt at breakpoints.')); |
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 use
$errorOutput->getStyle()->note()
instead. - Correct capitalization of
Xdebug
please: https://xdebug.org/ php-extension
should bePHP extension
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.
49add99
to
0280a3e
Compare
0280a3e
to
1f0abfc
Compare
Purrrrfect! |
Thank you. |
off-topic: you may also notice, that my commits from the work pc, now also use the same git user as those from my personal computer |
👏 |
The problem with the current warning message is that it can be interpreted as "please add the --xdebug flag to improve performance" which is wrong of course. See for example https://youtrack.jetbrains.com/issue/WI-69996 Please improve the message to help people understand that:
|
@InvisibleSmiley feel free to suggest a pull request with a concrete message. |
Shouldn't it be possible to disable this warning in the configuration file or something? It now breaks integrations with phpstan because the warning is always displayed. Setting 'start_with_request' to trigger still doesn't quiet the warning. So on a development machine where you want Xdebug to be available, you now always get this warning? Seems like it now breaks more than it helps? (The performance difference with XDEBUG_MODE=off vs XDEBUG_MODE=debug is very, very small). |
What does it break? It's printed to stderr. And there have always been messages printed to stderr, even when combined with JSON error format for example. |
I get a popup in phpstorm and vscode every time it tries to run phpstan, that there is some error running the process (and then shows the xdebug warning). |
|
I guess these ide plugins have ignored certain output to not complain about
other stderr output. Since i almost always need to specify a config file i
haven't encountered that situation.
In composer.json you can use ***@***.***` in a script definition to set the
XDEBUG_MODE variable I've discovered.
And in phpstorm you can add commandline options to the php interpreter used
for things like phpstan and php-cs-fixer.
The option `xdebug.mode=off` has the same effect as the env variable
(phpstorm uses this with a -d commandline parameter).
So I'm able to work around it for now.
Still seems weird to have to do this, as phpstan is often used during
development, where xdebug is almost always enabled.
Quite easy to see situations where you have little control over the command
used to invoke phpstan when it's inside a plugin or something like that .
Having config options to quiet all warnings still sound like something that
might eventually be necessary.
…On Thu, Dec 29, 2022, 10:05 Ondřej Mirtes ***@***.***> wrote:
1. The same integration should fail if there's a note in the output
about auto-discovered phpstan.neon or phpstan.neon.dist. Is that correct?
2. Seems like you can disable this message with an environment
variable XDEBUG_MODE=off. Is that correct?
—
Reply to this email directly, view it on GitHub
<#1878 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALMD2DW4DTOYDTH6DD62MVLWPVH5PANCNFSM6AAAAAARJ6BNC4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I guess this change breaks extensions like this? |
Extension that gets broken when something meant for humans to read is printed to |
Anyway, if you have any questions, please open new issues or discussions in phpstan/phpstan. Thank you. |
as discussed in phpstan/phpstan#8173
we don't have tests with/without xdebug. does this PR require tests?
I tested it manually