Skip to content
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

PHP: mark a breakpoint as broken when an error is received when breakpoint_set is executed to set a breakpoint #6876

Merged
merged 1 commit into from Dec 25, 2023

Conversation

troizet
Copy link
Collaborator

@troizet troizet commented Dec 23, 2023

xdebug of different versions behaves differently when setting duplicate breakpoints.
xdebug version 2.5.5 allows you to set a duplicate breakpoint.
xdebug version 3.1.6 and higher does not allow setting a duplicate breakpoint by method.
Since version 3.2.0 it also does not allow setting a duplicate breakpoint by line.

If an attempt is made to set a duplicate breakpoint, xdebug returns an error:

<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="breakpoint_set" transaction_id="141" state="enabled" status="starting" reason="ok">
	<error code="200">
		<message>
			<![CDATA[breakpoint could not be set]]>
		</message>
	</error>
</response>

This PR implements this error handling and marks the breakpoint as broken.
Tested on versions of xdebug: 2.5.5, 3.1.6, 3.2.1, 3.3.0.

Example for a line breakpoint:

  • Write code
<?php

echo 'Hi';

$s = 12;

echo 'Hello';
  • Put two breakpoints on the line echo 'Hello'; with different conditions (the order in which the breakpoints are set is important!):

    • first $s == 4
    • then $s == 12
  • When debugging on xdebug version 2.5.5 or 3.1.6, it will stop at the point with the condition $s == 12.
    Everything is fine with this.

  • When debugging on version 3.2.0, stopping at a point with the condition $s == 12 will not be executed, because the
    xdebug will not set the breakpoint. But the breakpoint is shown as normal, which is misleading.

Behavior before PR:

simplescreenrecorder-2023-12-24_00.46.34.mp4

Behavior after PR:

simplescreenrecorder-2023-12-24_00.44.03.mp4

@troizet troizet added the PHP [ci] enable extra PHP tests (php/php.editor) label Dec 23, 2023
@junichi11
Copy link
Member

It seems that there is an exception in your video. That is not related to this PR, right?
What was its behavior before? (couldn't start xdebug?) Thanks!

@junichi11 junichi11 added this to the NB21 milestone Dec 23, 2023
@troizet
Copy link
Collaborator Author

troizet commented Dec 23, 2023

Updated the description. I hope it will be clearer now.

@troizet
Copy link
Collaborator Author

troizet commented Dec 23, 2023

It seems that there is an exception in your video. That is not related to this PR, right?

Yes, that's right.

Copy link
Member

@tmysik tmysik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big code change, looks good to me. Thanks!

@junichi11
Copy link
Member

Updated the description. I hope it will be clearer now.

I see. Thanks for your explanation.

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

@troizet troizet force-pushed the php_mark_breakpoint_as_broken branch from e569ca5 to 1cda8a7 Compare December 25, 2023 17:38
@junichi11
Copy link
Member

Thank you, guys! Merging.

@junichi11 junichi11 merged commit e8b206e into apache:master Dec 25, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants