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.

Sign up for GitHub

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

Jump to bottom

Add DBGP proxy support, with client local port #378

Closed
wants to merge 35 commits into from
Closed

Add DBGP proxy support, with client local port #378

wants to merge 35 commits into from

Conversation

zobo
Copy link
Contributor

@zobo zobo commented Nov 15, 2019 •
edited by felixfbecker
Loading

Changes include option to specify an alternative ideport that can also be 0 and the system will choose a random port.

Includes #280

SidIcarus and others added 23 commits April 29, 2019 12:50
@SidIcarus
chore(settings.json): added markdownlint configs
85f74a5
@SidIcarus
style(README): formatted
fb1a942
@SidIcarus
docs(README): added proxy debugging info
2989a48 
-new supported launch.json settings
-feature: multi-user debugging,
-troubleshooting: run your own local proxy
@SidIcarus
docs(README): updated contribution steps and formatting
9d803b8
@SidIcarus
docs(package): added proxy args
edc55fb 
- allowMultipleSessions, enable, host, key, port
@towhidabsar @SidIcarus
feat(proxy): support for dbgp proxy connections
c835a0b 
- Implementation of ProxyConnect class.
- Upon initialization, will request registration with the proxy
- new launch options
  - allowMultipleSessions, enable, host, key, port
@towhidabsar @SidIcarus
test(proxy): unit tests
7b2e2f8
@SidIcarus
chore(tasks): use non-deprecated options
3c8ee4a
@SidIcarus
style(tasks): alphabetically sort options
6c448ce
@SidIcarus
fix: update ProcessEnv-esque properties to match interface
1c9a951 
- in addition, add `Non-null assertion operator` where appropriate
@SidIcarus
refactor(test): move retrieval of environment variables outside of tests
c12f28e 
- these aren't going to, nor should they, change while tests are running
@SidIcarus
style(README): move description below item
dc96534 
- has no affect on render
- easier viewability when editing the raw document collapsed
@SidIcarus
fix(README): incorrect badge link
922409a
@SidIcarus
style(readme): clean up shortcut url naming convention
3e01059
@SidIcarus
style(terminal): auto formatting
7f6a07a
@SidIcarus
feat(test): move travis before_install steps into their own file
a72d7ee 
- abstract out what the full php version is
- update hardcoded full php version in appveyor
- update base macos image
@SidIcarus
fix(proxyConnect): let msgs key types be inferred
182cda0
@SidIcarus
refactor(option): use bool v. num to improve clarity in the options
0c1ba35
@towhidabsar
fix(proxyConnect): add ProxyMessages interface and make socket pr…
8be4919 
…ivate

- new ProxyMessage interface to ensure typesafe messages
- socket is now private and can be passed through constructor
@towhidabsar
fix(proxyConnect): run prettier
cafeeec
@towhidabsar
fix(proxy): remove console.log
99ee211
@zobo
Send to proxy the IDE port, not the proxy port.
fe122c5
@zobo
If using proxy and no specific IDE port is set, let the OS choose a r…
63008f5 
…andom port and register that with the proxy.
@zobo zobo mentioned this pull request Nov 15, 2019
Closed
5 tasks
felixfbecker
felixfbecker reviewed Nov 15, 2019
View reviewed changes
src/phpDebug.ts Outdated Show resolved Hide resolved
@felixfbecker
Copy link
Contributor

The build is broken (test is failing). That would need to be fixed before merging.

(I'd like to merge this PR directly instead of the other if we know for a fact that the other one contains a bug)

@zobo
Copy link
Contributor Author

zobo commented Nov 17, 2019

Hey @felixfbecker. I used the better part of the weekend to get this working again. Turns out there was a strange conflict in php versions in the travis macos image. To be honest, I'd much rather just throw macos out of the pipeline. And also bring php and xdebug to a more recent and relevant version. I'll make another PR for that...

@felixfbecker felixfbecker reopened this Nov 17, 2019
@felixfbecker
Copy link
Contributor

Awesome, I'll take another look at the PR and then we can hopefully merge.

felixfbecker
felixfbecker requested changes Nov 17, 2019
View reviewed changes
Copy link
Contributor

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Unfortunately upon in-depth inspection I noticed that the ProxyConnect class has a couple issues that would make the proxy support not reliable compared to the direct connection. I think that needs to be solved before merging.

src/proxyConnect.ts Show resolved Hide resolved
src/proxyConnect.ts Outdated Show resolved Hide resolved
src/proxyConnect.ts Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
scripts/travis-beforeinstall.sh Outdated Show resolved Hide resolved
@zobo
Copy link
Contributor Author

zobo commented Nov 17, 2019

I actually agree with you in a lot of points, it's just that "I already got it like this". I will go over your suggestions and probably rewrite a lot of this code.
How do you feel about macos in travis? I think the scripts can be greatly simplified if we just make use of the provided php images (linux based...). I'd drop macos requirement in travis file.
Thank again for taking the time and being so critical.

@felixfbecker
Copy link
Contributor

Not having macOS tests makes me little bit uneasy, but it's been an absolute pain to maintain and configure and Travis takes forever on macOS. One option I was exploring was to use another CI provider with macOS support like Azure DevOps pipelines, but I'm not sure how much that would actually help. I wouldn't be opposed to remove it.

@zobo zobo force-pushed the port-fix branch 6 times, most recently from 8ee8e2a to 8e81d9c Compare November 22, 2019 22:11
@zobo
Updating PHP and XDebug versions for appveyor and travis, removing ma…
819b1b0 
…cos build. Removing unsupported windows build versions.
@elovin
Copy link

elovin commented Jun 20, 2020

@zobo there is a bit more active fork of this repository now, maybe you want to continue your work there ?

@MurzNN MurzNN mentioned this pull request Sep 28, 2020
Open
@MurzNN
Copy link

MurzNN commented Sep 28, 2020

Is there any progress with merging this PR into this repository, or into some forks like https://github.com/RobberPhex/vscode-php-debug?

@zobo
Copy link
Contributor Author

zobo commented Oct 5, 2020

Sorry about that, I was extremely busy lately. I’ll try to make a for for the other repo.

@felixfbecker felixfbecker changed the base branch from master to main December 21, 2020 20:20
@aldobarr
Copy link

@zobo It seems xdebug's dbgpProxy binary is not supported:

https://xdebug.org/docs/dbgpProxy
https://xdebug.org/download#dbgpProxy

@zobo
Copy link
Contributor Author

zobo commented Nov 3, 2021

Reimplemented in #685

@zobo zobo closed this Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfbecker felixfbecker Awaiting requested review from felixfbecker

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zobo @felixfbecker @elovin @MurzNN @aldobarr @SidIcarus @towhidabsar