PHP: New way to set the current breakpoint #6891

troizet · 2023-12-28T07:29:41Z

Now the logic of determining the current breakpoint is based on the information received from xdebug's response to the stack_get command, which is not always used to correctly determine the current breakpoint.

Since version 3.1.0 xdebug provides possibility to get information about current breakpoint.
To do this you need to enable breakpoint_details feature.
https://bugs.xdebug.org/bug_view_page.php?bug_id=00001969

In this PR I implemented the use of this feature, with fallback to the old logic, in case xdebug does not support this feature.

Examples before and after (using xdebug version 3.1.6):

Write code:

<?php

echo 'Hi!';

function x()
{
    echo 'Hello';
    return 12;
}

$x = x();

Set two method breakpoints on the x() function: one with stop on call, the other with stop on return

behavior before:

current_function_breakpoint_before.mp4

behavior after:

current_function_breakpoint_after.mp4

Write code:

<?php

echo 'Hi!';

class MethodBreakpoint
{
    public function x()
    {
            echo 'Hello';
            return 12;
    }
}

$cls = new MethodBreakpoint();

$x = $cls->x();

Set two method breakpoints on the x() method: one with stop on call, the other with stop on return

behavior before:

current_method_breakpoint_before.mp4

behavior after:

current_method_breakpoint_after.mp4

Write code:

<?php

echo 'Hi!';

$foo = $bar['foo'];

Set exception breakpoint on Notice

behavior before:

current_exception_breakpoint_before.mp4

behavior after:

current_exception_breakpoint_after.mp4

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

behavior before:

current_line_breakpoint_before.mp4

behavior after:

current_line_breakpoint_after.mp4

tmysik

Please, see my comment, thank you.

CC @junichi11

tmysik · 2023-12-28T13:39:06Z

php/php.dbgp/src/org/netbeans/modules/php/dbgp/breakpoints/BreakpointModel.java

@@ -54,6 +54,7 @@ public class BreakpointModel extends ViewModelSupport implements NodeModel {
    private static final String EXCEPTION = "TXT_Exception"; // NOI18N
    private static final String PARENS = "()"; // NOI18N
    private final Map<DebugSession, AbstractBreakpoint> myCurrentBreakpoints;
+    private Boolean searchCurrentBreakpointById = false;


Inconsistent threading - you are writing it under lock but read it without it. I guess that simple volatile should be fine in this case, so without any further locking.

tmysik · 2023-12-28T13:39:55Z

php/php.dbgp/src/org/netbeans/modules/php/dbgp/breakpoints/BreakpointModel.java

+        }
+    }
+
+    public Boolean isSearchCurrentBreakpointById() {


Suggested change

public Boolean isSearchCurrentBreakpointById() {

@CheckForNull

public Boolean isSearchCurrentBreakpointById() {

BTW, do we need to allow null for this field? I mean, cannot it be simply a boolean?

If I get it right, I would simply define it as boolean with default value false; and, if this feature is enabled/supported, it would be set to true. So, no need for null.

I agree with Tomas. If we use Boolean, we have to check null every time. It seems that null is not set. So, we can use boolean, I think (if I didn't overlook anything).

tmysik · 2023-12-28T13:41:34Z

php/php.dbgp/src/org/netbeans/modules/php/dbgp/packets/RunResponse.java

+        DebugSession.IDESessionBridge bridge = session.getBridge();
+        if (bridge != null) {
+            BreakpointModel breakpointModel = bridge.getBreakpointModel();
+            if (breakpointModel != null && breakpointModel.isSearchCurrentBreakpointById()) {


If I am not wrong, breakpointModel.isSearchCurrentBreakpointById() can produce a NPE, right?

tmysik · 2023-12-28T13:42:02Z

php/php.dbgp/src/org/netbeans/modules/php/dbgp/packets/StackGetResponse.java

@@ -96,7 +96,7 @@ private void updateBreakpointsView(DebugSession session, List<Stack> stacks) {
        IDESessionBridge bridge = session.getBridge();
        if (bridge != null) {
            BreakpointModel breakpointModel = bridge.getBreakpointModel();
-            if (breakpointModel != null) {
+            if (breakpointModel != null && !breakpointModel.isSearchCurrentBreakpointById()) {


If I am not wrong, breakpointModel.isSearchCurrentBreakpointById() can produce a NPE, right?

tmysik · 2023-12-28T13:46:47Z

php/php.dbgp/src/org/netbeans/modules/php/dbgp/packets/FeatureSetResponse.java

+            String feature = ((FeatureSetCommand) command).getFeature();
+            if (feature.equals(FeatureGetCommand.Feature.BREAKPOINT_DETAILS.toString())) {
+                Node error = getChild(getNode(), ERROR);
+                if (error != null) {


Nitpick: this whole condition can be perhaps a bit simplified:

setSearchCurrentBreakpointById(session, error == null);

tmysik · 2023-12-28T13:48:26Z

Thanks a lot for this PR! However, my knowledge of PHP DBG module is very limited, so I hope that @junichi11 will be able to review the change itself properly 😅

php/php.dbgp/src/org/netbeans/modules/php/dbgp/breakpoints/BreakpointModel.java

junichi11 · 2023-12-28T16:41:41Z

@troizet Thank you for your work and great description :) I agree with Tomas. So, could you have a look at the review?

troizet · 2023-12-28T17:21:19Z

Thank you so much for the review! I have made the suggested changes.

php/php.dbgp/src/org/netbeans/modules/php/dbgp/breakpoints/BreakpointModel.java

tmysik

Looks good to me now, thanks a lot! 👍

junichi11

Looks good to me. Thank you for your work!

junichi11 · 2023-12-30T12:32:58Z

Thank you, guys! Merging.

troizet · 2023-12-30T12:36:54Z

@tmysik, @junichi11 Thank you! Happy New Year!

tmysik · 2023-12-30T14:02:15Z

@troizet @junichi11

Happy New Year to you too!

junichi11 · 2023-12-30T16:25:08Z

@troizet @tmysik Happy New Year to you too!

troizet added the PHP [ci] enable extra PHP tests (php/php.editor) label Dec 28, 2023

troizet requested review from tmysik and junichi11 December 28, 2023 08:14

tmysik requested changes Dec 28, 2023

View reviewed changes

tmysik reviewed Dec 28, 2023

View reviewed changes

junichi11 reviewed Dec 28, 2023

View reviewed changes

php/php.dbgp/src/org/netbeans/modules/php/dbgp/breakpoints/BreakpointModel.java Outdated Show resolved Hide resolved

junichi11 added this to the NB21 milestone Dec 28, 2023

troizet force-pushed the php_improve_set_current_breakpoint branch from a38e13e to 5110799 Compare December 28, 2023 17:19

junichi11 reviewed Dec 28, 2023

View reviewed changes

php/php.dbgp/src/org/netbeans/modules/php/dbgp/breakpoints/BreakpointModel.java Outdated Show resolved Hide resolved

PHP: New way to set the current breakpoint

b006691

troizet force-pushed the php_improve_set_current_breakpoint branch from 5110799 to b006691 Compare December 30, 2023 03:47

tmysik approved these changes Dec 30, 2023

View reviewed changes

junichi11 approved these changes Dec 30, 2023

View reviewed changes

junichi11 merged commit a34e570 into apache:master Dec 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP: New way to set the current breakpoint #6891

PHP: New way to set the current breakpoint #6891

troizet commented Dec 28, 2023 •

edited

Loading

tmysik left a comment

tmysik Dec 28, 2023

tmysik Dec 28, 2023

tmysik Dec 28, 2023

tmysik Dec 28, 2023

junichi11 Dec 28, 2023

tmysik Dec 28, 2023

tmysik Dec 28, 2023

tmysik Dec 28, 2023

tmysik commented Dec 28, 2023

junichi11 commented Dec 28, 2023

troizet commented Dec 28, 2023 •

edited

Loading

tmysik left a comment

junichi11 left a comment

junichi11 commented Dec 30, 2023

troizet commented Dec 30, 2023

tmysik commented Dec 30, 2023

junichi11 commented Dec 30, 2023

	public Boolean isSearchCurrentBreakpointById() {
	@CheckForNull
	public Boolean isSearchCurrentBreakpointById() {

PHP: New way to set the current breakpoint #6891

PHP: New way to set the current breakpoint #6891

Conversation

troizet commented Dec 28, 2023 • edited Loading

tmysik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmysik commented Dec 28, 2023

junichi11 commented Dec 28, 2023

troizet commented Dec 28, 2023 • edited Loading

tmysik left a comment

Choose a reason for hiding this comment

junichi11 left a comment

Choose a reason for hiding this comment

junichi11 commented Dec 30, 2023

troizet commented Dec 30, 2023

tmysik commented Dec 30, 2023

junichi11 commented Dec 30, 2023

troizet commented Dec 28, 2023 •

edited

Loading

troizet commented Dec 28, 2023 •

edited

Loading