Skip to content

PhpFpmDebug #60

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

dsmolovich
Copy link

@dsmolovich dsmolovich commented Dec 8, 2019

  1. Introduces phpfpmdebug service, nginx routes calls to this service based on whether the XDEBUG_SESSION cookie is set. This speeds up the development process as there's no need to run debug-on & debug-off commands anymore.
    See for more details: https://jtreminio.com/blog/developing-at-full-speed-with-xdebug/

  2. Makes XDebug hitting PhpStorm's default 9000 port.

In overall this makes the developer's experience more pleasant as it won't require:
a) restarting the stack before and after the debugging session (= eliminates slow downs to the bare minimum)
b) changing the listening port in PhpStorm settings

P.S.

@sanaeefar-saeed
Copy link

It's extremely useful! I found it a couple of months ago.
I've used the same instruction you mentioned (https://jtreminio.com/blog/developing-at-full-speed-with-xdebug/) with a little bit different in implementation.
Anyway, it works like a charm :)

@@ -41,7 +48,7 @@ server {
root $MAGE_ROOT;
location ~ ^/setup/index.php {
fastcgi_split_path_info ^(.+\.php)(/.+)$;
fastcgi_pass fastcgi_backend;
fastcgi_pass $my_fastcgi_pass:9001;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need 9001 port here? As it's not php-fpm node -it's not conflicting anyhow with default 9000 port of xdebug.

Could you replace it to 9000?

Suggested change
fastcgi_pass $my_fastcgi_pass:9001;
fastcgi_pass $my_fastcgi_pass:9000;

Related to #49

;user = app
;group = app

listen = 9001
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
listen = 9001
listen = 9000

volumes: *appvolumes-mac

phpfpmdebug:
volumes:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we could re-use volumes from phpfpm service. Can't we?

phpfpm:
image: modestcoders/php:7.1-fpm

nginx:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't thing we really need to flip phpfom and nginx. Is there any idea behind it?

@ihor-sviziev
Copy link
Contributor

@dsmolovich really good idea! Thank you!

@dsmolovich
Copy link
Author

@ihor-sviziev The reason for the nginx/phpfpm swap was that I used to define graphs with dependencies going upwards or downwards (in this case it was downward: nginx depends on phpfpm and phpfpmdebug) - easier for reading and understanding. It's back now.

why do we need 9001 port here? As it's not php-fpm node -it's not conflicting anyhow with default 9000 port of xdebug.
Could you replace it to 9000?

Agree. There was no real reason for this, it's back now.

I believe we could re-use volumes from phpfpm service. Can't we?

No luck. Unfortunately I have no idea on how to merge alias with other list elements. :)

BTW, it's the fastest mage env I've ever seen, I had no idea m2 can be that quick. Thank you for this project guys!

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Looks like linux docker-compose file should be also updated (BTW not sure about windows)

- db
- elasticsearch

phpfpmdebug:
image: modestcoders/php:7.2-fpm
Copy link
Contributor

Choose a reason for hiding this comment

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

image should be the same as in phpfpm: modestcoders/php:7.1-fpm
otherwise we'll have different php versions with and w/o xdebug

@@ -4,23 +4,30 @@ services:
phpfpm:
image: modestcoders/php:7.1-fpm
volumes: &appvolumes
- sockdata:/sock
Copy link
Contributor

Choose a reason for hiding this comment

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

now sockdata volume is never used, probably could be removed

@@ -26,6 +22,9 @@ services:
- ./config/dockergento/usr/local/etc/php/conf.d/docker-php-ext-xdebug.ini:/usr/local/etc/php/conf.d/docker-php-ext-xdebug.ini:delegated
Copy link
Contributor

Choose a reason for hiding this comment

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

There is missing # {FILES_IN_GIT} - it will be replaced during setup

@danielozano
Copy link
Member

Ok it looks like a good idea atm.
I see a lot of changes that should be done into dockerfiles, but it's hard to maintain this kind of features with 2 different repositories...

I'm giving it a try, so I'll merge it if I don't see any issues (I can only test it on linux at the moment). Later I will move changes to dockerfiles!

I've only 1 question (until I try it). In this line we expose port 9001: https://github.com/ModestCoders/dockerfiles/blob/master/php/7.2-fpm/Dockerfile#L70
That line could be deleted right?

Thank you for your work! @ihor-sviziev @dsmolovich

@ihor-sviziev
Copy link
Contributor

Yes, we can remove it

@killua99
Copy link

This wasn't ready to merge? Waiting for docker images to update?

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.

5 participants