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

Updated docs for CLI debugging with drush #2318

Merged
merged 3 commits into from
Jun 23, 2020

Conversation

chellman
Copy link
Contributor

The Problem/Issue/Bug:

I had some trouble successfully connecting drush, xdebug, and PhpStorm. The PHP_IDE_CONFIG setting wasn't exactly what I needed.

How this PR Solves The Problem:

The docs are updated to note that PHP_IDE_CONFIG should be exported as export PHP_IDE_CONFIG=serverName=<phpstorm_project_name>. I added this in a couple other places where it seemed to make sense. I also updated the drush.example command with a comment noting that you can execute with the full path to drush instead of using the launcher to potentially make this connection more reliable.

Manual Testing Instructions:

n/a

Automated Testing Overview:

Just docs and a comment in a command, no tests needed.

Related Issue Link(s):

None explicitly, but maybe #1648 is a little bit relevant.

Release/Deployment notes:

n/a

@rfay
Copy link
Member

rfay commented Jun 18, 2020

Thanks!

@@ -26,6 +26,7 @@ version: '3.6'
services:
web:
environment:
- PHP_IDE_CONFIG=serverName=mysite.ddev.site
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having to add it custom every time, couldn't we add it to the list of default env variables generated by ddev?

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I already add this customization to all my ddev containers, so +1 on its addition. The only question is about implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea, but I don’t think this can be automated because the value depends on what you‘ve set up for the server name in PhpStorm. It’s not the host name that ddev generates (though of course they could be the same).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess what I'm suggesting is why not default it to the ddev generated name? Its what I've been using. Naming things is hard so I just use the name provided so I don't have to think of a new one.

Copy link
Member

Choose a reason for hiding this comment

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

Note that PHP_IDE_CONFIG can easily be set in the bash session inside the container. That's my own preference. I just set it when I need it. And yes $SITENAME (I think that's what's needed) would be better here than mysite.ddev.site.

Copy link
Member

Choose a reason for hiding this comment

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

maybe PHP_IDE_CONFIG should be added as an env in the code, rather than having to do any of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I'm happy wherever it goes as long as it's documented. I'd love it if this were deployed now since that's how it is right now, and then move the config to a better place, if needed, separately.

@rfay rfay force-pushed the 20200617-xdebug-cli-phpstorm branch from cdde2bf to 06875e1 Compare June 22, 2020 22:51
@rfay
Copy link
Member

rfay commented Jun 22, 2020

I went ahead added PHP_IDE_CONFIG always (with the default serverName).

I also rebased this against current, so if you have any more commits, make sure to reset against the fork before continuing.

I hope that both of you can take a look at this. If you want, you can take a look at it in upcoming alpha5, but you can also download the artifacts from this PR once the build is done. If you want to do that I'll get the link.

Copy link
Collaborator

@heddn heddn left a comment

Choose a reason for hiding this comment

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

One small question/nit. Otherwise, this looks really nice.

# To use the site-installed drush directly (rather than running /usr/local/bin/drush),
# you might want to execute it directly:
#
# /var/www/html/vendor/bin/drush $@
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to use the relative path i.e. vendor/bin/drush. Not sure if that is just me or if other do that too.

Copy link
Member

Choose a reason for hiding this comment

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

Just bare vendor/bin/rush doesn't work normally with drupal8+ project type because the working directory is /var/www/html/web, so it needs to be ../vendor/bin/drush in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the site is using any fairly modern version of drush, then it should work. Its worked from above the web docroot for a couple years now. I know it didn't used to, but that hasn't been true for a long time now.

Copy link
Member

Choose a reason for hiding this comment

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

Here's the problem:

$ ddev exec pwd
/var/www/html/web

When using ddev exec with a Drupal project, the working directory inside the container is the docroot (explicitly to make drush work on older versions of Drupal, so you're right there, it could be changed for drupal8+, but not in this PR). But ddev exec on drupal executes in the docroot, meaning that the relative path to site-local drush is not vendor/bin/drush, it would have to be ../vendor/bin/drush, and that's pretty ambiguous.

But I do agree that we could change the working directory in drupal8+

@@ -111,6 +111,7 @@ services:
- COLUMNS=$COLUMNS
- LINES=$LINES
- TZ={{ .Timezone }}
- PHP_IDE_CONFIG=serverName=${DDEV_SITENAME}.${DDEV_TLD}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -71,7 +71,7 @@ services:
entrypoint: [ "sh", "-c", "docker-entrypoint.sh solr-precreate dev /solr-conf" ]

external_links:
- "ddev-router:${DDEV_SITENAME}.ddev.site"
- "ddev-router:${DDEV_SITENAME}.${DDEV_TLD}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I assume this is the only other place this surfaces?

Copy link
Member

Choose a reason for hiding this comment

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

I tried to look carefully. That's just an example file, but I was surprised to see the "ddev.site" wired in there. I'll bet there are several more in ddev-contrib.

@rfay
Copy link
Member

rfay commented Jun 23, 2020

If you are OK with it @chellman we'll pull it into alpha5.

@chellman
Copy link
Contributor Author

@rfay Yes, I think this looks good. I kind of want that "e.g." at the end of the line in drush.example so it's totally clear that the path might be different, but that's my only thought. Otherwise, go go go!

@rfay rfay merged commit 0c5a034 into ddev:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants