GeezerVison is showing github.com — fix: do not enable debugger if notifications are on by carlos-granados · Pull Request #95 · php-debugger/php-debugger
Conversation
One of the conditions we were checking to see if we could disable the debugger after a connection had been made was if the notifications were enabled. The problem is that PhpStorm sends a NOTIFY_OK command when connecting, so this will always be on when connecting to PhpStorm and this would negate our optimizations for this case. But turns out that we don't really need to enable the debugger just because the notifications are on as they are only checked in cases where the debugger would stop. So this check can be removed and the debugger will be performant under PhpStorm again
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Makes sense to me but would be nice to add a test perhaps?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@claude add test to cover the described case
|
claude[bot] reacted with eyes emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
claude
Bot
commented
Jun 29, 2026
•
edited by carlos-granados
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Claude finished @pronskiy's task in 6m 26s —— View job
Task: Add test to cover the described case
- Read PR diff and understand the fix
- Explore existing tests (especially
rebuild-stack-after-xdebug-break.phpt, the test added with the original optimisation in feature: optimize the case where we are connected but not actively used #65)
- Write a new test that mirrors the scenario but with
notify_ok=1 (PhpStorm-style)
- Commit locally
- Push to PR branch — blocked: GitHub Actions can't push to forks (this PR is from
carlos-granados's fork, push returned 403)
What the test covers
The fix removes send_notifications from the guard in xdebug_dbgp_cmdloop so the observer/statement-handler is now uninstalled at the end of run even when notify_ok=1. The new test exercises the exact path PhpStorm hits:
feature_set notify_ok 1 (what PhpStorm sends on connect — used to disable the optimisation pre-fix).xdebug.on_demand_debugging_enabled=0 (the condition under which the observer is fully uninstalled — pre-fix this would also be inhibited by send_notifications).run — under the new code the observer is uninstalled here; under the old code it stayed installed.- The script then hits
xdebug_break(), which has to rebuild the stack before reporting the break.
- Subsequent
stack_get / step_into / step_out / stack_get assertions verify the rebuilt stack and stepping behaviour are still correct.
This mirrors tests/debugger/rebuild-stack-after-xdebug-break.phpt (added in #65 to lock the original optimisation) — same script, same expected behaviour, just with notifications enabled.
Files (committed locally as 420f35f but not pushable from this runner)
tests/debugger/rebuild-stack-after-xdebug-break-with-notifications.inc
<?php
function inner_function() {
return 42;
}
function outer_function() {
xdebug_break();
$result = inner_function();
return $result;
}
outer_function();tests/debugger/rebuild-stack-after-xdebug-break-with-notifications.phpt
--TEST--
Stack is correctly rebuilt after xdebug_break() with on_demand_debugging_enabled=0 and notifications enabled
--SKIPIF--
<?php
require __DIR__ . '/../utils.inc';
check_reqs('dbgp');
?>
--FILE--
<?php
require 'dbgp/dbgpclient.php';
$filename = dirname(__FILE__) . '/rebuild-stack-after-xdebug-break-with-notifications.inc';
$commands = array(
'feature_set -n notify_ok -v 1',
'run',
'stack_get',
'step_into',
'stack_get',
'step_out',
'stack_get',
'detach',
);
$settings = [
'xdebug.mode' => 'debug',
'xdebug.start_with_request' => 'yes',
'xdebug.on_demand_debugging_enabled' => 0
];
dbgpRunFile( $filename, $commands, $settings );
?>
--EXPECTF--
<?xml version="1.0" encoding="iso-8859-1"?>
<init xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" fileuri="file://rebuild-stack-after-xdebug-break-with-notifications.inc" language="PHP" xdebug:language_version="" protocol_version="1.0" appid=""><engine version=""><![CDATA[PHP Debugger]]></engine><author><![CDATA[Derick Rethans]]></author><url><![CDATA[https://xdebug.org]]></url><copyright><![CDATA[Copyright (c) 2002-2099 by Derick Rethans]]></copyright></init>
-> feature_set -i 1 -n notify_ok -v 1
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="feature_set" transaction_id="1" feature="notify_ok" success="1"></response>
-> run -i 2
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="run" transaction_id="2" status="break" reason="ok"><xdebug:message filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="8"></xdebug:message></response>
-> stack_get -i 3
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="stack_get" transaction_id="3"><stack where="outer_function" level="0" type="file" filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="8"></stack><stack where="{main}" level="1" type="file" filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="12"></stack></response>
-> step_into -i 4
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="step_into" transaction_id="4" status="break" reason="ok"><xdebug:message filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="3"></xdebug:message></response>
-> stack_get -i 5
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="stack_get" transaction_id="5"><stack where="inner_function" level="0" type="file" filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="3"></stack><stack where="outer_function" level="1" type="file" filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="8"></stack><stack where="{main}" level="2" type="file" filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="12"></stack></response>
-> step_out -i 6
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="step_out" transaction_id="6" status="break" reason="ok"><xdebug:message filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="9"></xdebug:message></response>
-> stack_get -i 7
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="stack_get" transaction_id="7"><stack where="outer_function" level="0" type="file" filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="9"></stack><stack where="{main}" level="1" type="file" filename="file://rebuild-stack-after-xdebug-break-with-notifications.inc" lineno="12"></stack></response>
-> detach -i 8
<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="detach" transaction_id="8" status="stopping" reason="ok"></response>
Notes & caveats
- I wasn't able to build the extension on this runner (no
phpize/build permissions here), so I couldn't run the test locally. The --EXPECTF-- block is derived from the existing rebuild-stack-after-xdebug-break.phpt (identical script, identical commands shifted by one transaction id, plus the leading feature_set exchange copied from xdebug_notify-001.phpt). It should be byte-for-byte correct, but please run it through run-xdebug-tests.php before merging in case anything needs a tweak.
- @carlos-granados — since the runner can't push to the fork branch, you'll need to add these two files to the PR. Happy to iterate if the EXPECTF needs adjustment after a local run.
· branch fix/do-not-enable-debugger-for-notifications
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
pronskiy
pronskiy approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|