-
Notifications
You must be signed in to change notification settings - Fork 350
[ xdebug ] Add --experimental-ide
option in Playground CLI
#2777
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
base: trunk
Are you sure you want to change the base?
Conversation
--experimental-ide
option to set xdebug path mappings--experimental-ide
option in Playground CLI
Let's support an optional explicit value, e.g. Also, this is the top of my
It means we cannot trivially use |
try { | ||
config = JSON.parse(fs.readFileSync(configFilePath, 'utf-8')); | ||
} catch { | ||
logger.warn( |
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 is a big deal, let's display it in red and kill the process at this point. Otherwise, when things don't work, the user will scratch their head and think "oh that option doesn't work?"
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 used jsonc-parser
as suggested in your comment, and it became easier to manipulate JSON.
Now I parse the file and if it finds errors, it logs an error and exits with process.exit(1)
.
My question is, should I use the process.exit() function inside the xdebug-path-mappings file or should I return to the run-cli
file to centralize process.exit functions ?
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.
Good question! Would throw new Error() suffice? AFAIR CLI has an error handler that already calls exit()
// Then, if xdebug, and experimental IDE are enabled, | ||
// recreate the symlink pointing to the temporary | ||
// directory and add the new IDE config. | ||
if (args.xdebug && args.experimentalIde) { |
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.
cc @brandonpayton for reviews
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.
@brandonpayton and @adamziel, your reviews are warmly welcomed in this pull request, since I am not sure I will list all the use cases.
(c: { $: { name: string } }) => c.$.name === name | ||
); | ||
if (!servers) { | ||
component.servers[0].server.push(server); |
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 line keep failing for me, every part of this expression may be missing (like it was in my config).
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 had this same experience and believe I've fixed it now.
); | ||
if (!component) { | ||
config.project.component = []; | ||
config.project.component.push({ |
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 we set component
to the pushed element?
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 be fixed now.
XDebug is running even during the Blueprint execution – how cool! Also, that's likely only useful for code developers. Let's set some php.ini option to disable xdebug until WordPress fully boots. |
I've been testing and reviewing the code and made some adjustments. The server host an port are now explicitly configured based on CLI's host and port. I've seen PhpStorm report an Xdebug connection but haven't been able to hit any breakpoints yet. |
I've been testing with an auto-mounted WordPress dir nested under the Playground repo like: |
@adamziel is there any additional step in PhpStorm that I should have to do to tell PhpStorm to be able to hit breakpoints? Or should it just work if you have PhpStorm open and Playground CLI started with Xdebug support? |
I typically check "stop on first line" and set breakpoints from there @brandonpayton. And you need to turn on the "accept debug connections" toggle |
Thanks, @adamziel! I hadn't been testing with "Stop on First Line". For some reason, PhpStorm still isn't breaking in the UI. I grabbed WireShark to capture the interaction between Playground CLI and the IDE, and based on my naive reading of the exchange, it appears to be trying to break (or declaring a break?) for auto_prepend_file.php but then ends with a "detach" command before doing anything within the UI. I am thinking about why this may be, including considering whether I broke anything with fixes over the weekend. WireShark Logs from the TCP stream493.<?xml version="1.0" encoding="iso-8859-1"?>
<init xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" fileuri="file:///internal/shared/auto_prepend_file.php" language="PHP" xdebug:language_version="8.3.26-dev" protocol_version="1.0" appid="42"><engine version="3.4.6-dev"><![CDATA[Xdebug]]></engine><author><![CDATA[Derick Rethans]]></author><url><![CDATA[https://xdebug.org]]></url><copyright><![CDATA[Copyright (c) 2002-2025 by Derick Rethans]]></copyright></init>.
eval -i 1 -- KHN0cmluZykoaW5pX2dldCgneGRlYnVnLmNvdmVyYWdlX2VuYWJsZScpLic7Jy5pbmlfZ2V0KCd4ZGVidWcucHJvZmlsZXJfZW5hYmxlJykuJzsnLmluaV9nZXQoJ3hkZWJ1Zy5yZW1vdGVfYXV0b3N0YXJ0JykuJzsnLmluaV9nZXQoJ3hkZWJ1Zy5yZW1vdGVfY29ubmVjdF9iYWNrJykuJzsnLmluaV9nZXQoJ3hkZWJ1Zy5yZW1vdGVfbW9kZScpKQ==.feature_set -i 2 -n show_hidden -v 1.
1102.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="eval" transaction_id="1"><property type="string" size="635" encoding="base64"><![CDATA[VGhpcyBzZXR0aW5nIGhhcyBiZWVuIGNoYW5nZWQsIHNlZSB0aGUgdXBncmFkaW5nIGd1aWRlIGF0IGh0dHBzOi8veGRlYnVnLm9yZy9kb2NzL3VwZ3JhZGVfZ3VpZGUjY2hhbmdlZC14ZGVidWcuY292ZXJhZ2VfZW5hYmxlO1RoaXMgc2V0dGluZyBoYXMgYmVlbiBjaGFuZ2VkLCBzZWUgdGhlIHVwZ3JhZGluZyBndWlkZSBhdCBodHRwczovL3hkZWJ1Zy5vcmcvZG9jcy91cGdyYWRlX2d1aWRlI2NoYW5nZWQteGRlYnVnLnByb2ZpbGVyX2VuYWJsZTtUaGlzIHNldHRpbmcgaGFzIGJlZW4gY2hhbmdlZCwgc2VlIHRoZSB1cGdyYWRpbmcgZ3VpZGUgYXQgaHR0cHM6Ly94ZGVidWcub3JnL2RvY3MvdXBncmFkZV9ndWlkZSNjaGFuZ2VkLXhkZWJ1Zy5yZW1vdGVfYXV0b3N0YXJ0O1RoaXMgc2V0dGluZyBoYXMgYmVlbiBjaGFuZ2VkLCBzZWUgdGhlIHVwZ3JhZGluZyBndWlkZSBhdCBodHRwczovL3hkZWJ1Zy5vcmcvZG9jcy91cGdyYWRlX2d1aWRlI2NoYW5nZWQteGRlYnVnLnJlbW90ZV9jb25uZWN0X2JhY2s7VGhpcyBzZXR0aW5nIGhhcyBiZWVuIGNoYW5nZWQsIHNlZSB0aGUgdXBncmFkaW5nIGd1aWRlIGF0IGh0dHBzOi8veGRlYnVnLm9yZy9kb2NzL3VwZ3JhZGVfZ3VpZGUjY2hhbmdlZC14ZGVidWcucmVtb3RlX21vZGU=]]></property></response>.219.<?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="2" feature="show_hidden" success="1"></response>.
feature_set -i 3 -n max_depth -v 1.
217.<?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="3" feature="max_depth" success="1"></response>.
feature_set -i 4 -n max_children -v 100.
220.<?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="4" feature="max_children" success="1"></response>.
feature_set -i 5 -n extended_properties -v 1.
227.<?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="5" feature="extended_properties" success="1"></response>.
feature_set -i 6 -n notify_ok -v 1.
217.<?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="6" feature="notify_ok" success="1"></response>.
feature_set -i 7 -n resolved_breakpoints -v 1.
228.<?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="7" feature="resolved_breakpoints" success="1"></response>.
feature_set -i 8 -n breakpoint_include_return_value -v 1.
239.<?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="8" feature="breakpoint_include_return_value" success="1"></response>.
stdout -i 9 -c 1.
192.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="stdout" transaction_id="9" success="1"></response>.
status -i 10.
211.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="status" transaction_id="10" status="starting" reason="ok"></response>.
step_into -i 11.
312.<?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="11" status="break" reason="ok"><xdebug:message filename="file:///internal/shared/auto_prepend_file.php" lineno="3"></xdebug:message></response>.
eval -i 12 -- Z2V0ZW52KCdQSFBfSURFX0NPTkZJRycpIT1mYWxzZQ==.
225.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="eval" transaction_id="12"><property type="bool"><![CDATA[0]]></property></response>.
eval -i 13 -- aXNzZXQoJF9TRVJWRVJbJ1NFUlZFUl9OQU1FJ10p.
225.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="eval" transaction_id="13"><property type="bool"><![CDATA[1]]></property></response>.
eval -i 14 -- KHN0cmluZykoJF9TRVJWRVJbJ1NFUlZFUl9OQU1FJ10p.
274.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="eval" transaction_id="14"><property type="string" size="15" encoding="base64"><![CDATA[ZXhhbXBsZS5jb206NDQz]]></property></response>.
eval -i 15 -- KHN0cmluZykoJF9TRVJWRVJbJ1NFUlZFUl9QT1JUJ10p.
257.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="eval" transaction_id="15"><property type="string" size="2" encoding="base64"><![CDATA[ODA=]]></property></response>.
eval -i 16 -- KHN0cmluZykoJF9TRVJWRVJbJ1JFUVVFU1RfVVJJJ10p.
253.<?xml version="1.0" encoding="iso-8859-1"?>
<response xmlns="urn:debugger_protocol_v1" xmlns:xdebug="https://xdebug.org/dbgp/xdebug" command="eval" transaction_id="16"><property type="string" size="0" encoding="base64"><![CDATA[]]></property></response>.
detach -i 17 -- Q2Fubm90IGJpbmQgZmlsZSAvaW50ZXJuYWwvc2hhcmVkL2F1dG9fcHJlcGVuZF9maWxlLnBocCB0byB0aGUgd2ViIHNlcnZlciBwcm9qZWN0LiBGb3IgbW9yZSBpbmZvcm1hdGlvbiwgcmVmZXIgdG8gaHR0cHM6Ly93d3cuamV0YnJhaW5zLmNvbS9oZWxwL3BocHN0b3JtL3Ryb3VibGVzaG9vdGluZy1waHAtZGVidWdnaW5nLmh0bWwjZGV0YWNoLWZyb20tZGVidWctc2Vzc2lvbi1yZWFzb25zLg==.
211.<?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="17" status="stopping" reason="ok"></response>.
stop -i 18.
|
Funny. I "fixed" this over the weekend but manually changing back to the above setting fixed the issue for me. I've hit my first breakpoint in PhpStorm. 🎉 |
I'm going to test with VSCode next. If that is working, I want to find the simplest version of this we can ship and then continue iterating. |
After fixing a race condition between clear-ide-config and add-ide-config, the VSCode setup works great. I was able to hit breakpoints with no problem. Now, I'm reviewing the code to clean things up and see whether we are in a place where we can merge this and follow-up with a PR to further refine the feature. |
I kept running into bugs with different special cases in the To make sure element order was preserved (the respectful thing to do IMO) and to make the parsing result more consistent (and thus easier to think about), I adjusted the xml2js parser options so that every element would have a I also noticed that xml2js was replacing CDATA sections with text nodes. This might have been fixable by changing xml2js options, but at that point, I was already pursuing a less magical parser/builder. I moved to I see that we are also using xml2js with the xdebug-cdp-bridge, but I think that is fine for now. And if we want, we can switch to a single XML library again in the future. |
Note: I updated the previous comment with additional info. |
I plan to continue looking at this in the morning. After moving to the new parser, the PhpStorm config parsing and modification seems much more stable. I'm troubled a bit that this feature could complete screw up a person's IDE config if there are bugs and am considering ways we might avoid this. One idea: Maybe what we are doing now will be OK, but the above concern is on my mind. |
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.
@adamziel, I think this is ready for reviews.
Regarding the fact that we can hit breakpoints during boot, it seems like we can handle that with a change to the Xdebug feature itself in a follow-up PR.
// TODO: Should we warn users and ask them to confirm that | ||
// we will be modifying their IDE config files? | ||
// It could be painful for folks if their IDE configs are | ||
// inadvertently broken. |
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.
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.
As a user, I would appreciate such a prompt with a warning that this carries some risk, and it might not be bad to ask if they want Playground CLI to create a backup.
Though, after the initial prompt, I'd probably want a way to tell Playground CLI not to prompt me.
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.
What if we name this feature something like --unsafe-ide-integration
or something? I'd rather not have a name this dramatic, but maybe it would be good for the option name include some acknowledgement of the risk.
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 unsafe
in the name. Also, we need a prompt-less mode at least for integrators. Perhaps based on the option value? Default could be ask
and there could be another value available such as dont-ask
if (projectElement === undefined) { | ||
projectElement = { | ||
project: [], | ||
':@': { version: '4' }, |
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.
Do we want to tie ourselves to a specific version number here, and if so, should we refuse to modify configs with unrecognized project versions?
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.
Yeah let's tie ourselves to a specific version. We have no idea what version 5 will look like. Also, it seems like 4 was around at least since 2017 so maybe it will stick around for another 8 years.
const symlinkName = '.playground'; | ||
const symlinkPath = path.join(process.cwd(), symlinkName); | ||
|
||
removePlaygroundCliTempDirSymlink(symlinkPath); |
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.
Rare but possible case:
What if a user project already includes a .playground
dir?
Maybe we should name the symlink .playground-files-for-debugging
or something else specific.
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.
Good spot, I think some projects on GitHub include a directory like that. .playground-xdebug-root
perhaps?
It's late. I don't want to create another issue right now, so I'll just note this here for now: I think we can fix this case in a separate PR by:
And if we want to enable devs to use Xdebug during boot, we can support a separate command line flag for that. |
Thanks for your responses, @adamziel. They all make sense, and I should be able to implement them in the morning. |
Motivation for the change, related issues
Based on issue #2763
The current pain we have when we run a Playground CLI command with Xdebug enabled is the absence of the files located in the VFS in our IDE.
This pull request gives visibility to the VFS files with the help of a
.playground
symlink and the generation of path mappings inside the developer's IDE.Only the mounts from inside the current working directory are mapped.
Implementation details
.playground
.playground
in the current working directory.--xdebug
and--experimental-ide
options are present, we symlink the temporary playground cli directory inside the current working directory.WP Playground CLI - Listen for Xdebug
in VSCode and PHPStorm config files.--xdebug
and--experimental-ide
options are present, we add IDE configs and path mappings in the related configs.server
with nameWP Playground CLI - Listen for Xdebug
in.idea/workspace.xml
.configuration
with nameWP Playground CLI - Listen for Xdebug
in.vscode/launch.json
.Next
Testing Instructions (or ideally a Blueprint)
CI