-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add separate phpfpm container dedicated to xdebug #1355
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: master
Are you sure you want to change the base?
feat: add separate phpfpm container dedicated to xdebug #1355
Conversation
The changes contains: - drop xdebug from current php-fpm container - add new php-fpm container with xdebug configuration - changes to nginx to name sockets accordingly and apply mapping to do it based on cookie in request
PR Summary
|
# image: markoshust/magento-nginx:1.24-0 | ||
build: | ||
context: ./images/nginx/1.24 | ||
dockerfile: Dockerfile |
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.
@markshust This should be replaced with dedicated image build
# image: markoshust/magento-php:8.3-fpm-4 | ||
build: | ||
context: ./images/php/8.3 | ||
dockerfile: Dockerfile |
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.
@markshust This should be replaced with dedicated image build
# image: markoshust/magento-php:8.3-fpm-4 | ||
build: | ||
context: ./images/php/8.3-xdebug | ||
dockerfile: Dockerfile |
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.
@markshust This should be replaced with dedicated image build
server unix:/sock/phpfpm-xdebug.sock; | ||
} | ||
|
||
map $cookie_XDEBUG_SESSION $fastcgi_backend { |
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.
The problem I mentioned in the description is that mapping requires to put $
sign when assigned to variable. There is a logic which copies nginx.conf.sample
to nginx.conf
but this sample file has
fastcgi_pass fastcgi_backend;
but it should be
fastcgi_pass $fastcgi_backend;
The occurence is repeated 3 times - perhaps some post-script with sed
that would replace it?
# curl -s https://raw.githubusercontent.com/markshust/docker-magento/master/lib/template | bash | ||
curl -s https://raw.githubusercontent.com/Jarzebowsky/docker-magento/xdebug-change-trigger-method/lib/template | bash |
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 change was introduced to fully test the approach so it should be reverted.
It's left for the sake of testing it locally.
# git remote add origin https://github.com/markshust/docker-magento | ||
git remote add origin https://github.com/Jarzebowsky/docker-magento | ||
git fetch origin -qqq | ||
git checkout origin/master -- compose | ||
# git checkout origin/master -- compose | ||
git checkout origin/xdebug-change-trigger-method -- compose images | ||
|
||
mv compose/* ./ | ||
mv compose/.gitignore ./ | ||
mv compose/.vscode ./ | ||
mv images/* ./images |
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 change was introduced to fully test the approach so it should be reverted.
It's left for the sake of testing it locally.
It should probably also include the changes to the README (didn't do anything as the initial approve should be happening before that happens).
There is also one thing that I don't have idea how to solve as it requires manual change after setting up the project - perhaps some core contributors have better understanding of the repository to drop the lead or give concrete approach for that.
Context
This resolves #1230 which drops manual interaction with turning on/off xdebug which is problematic to remember for a user but also even if it's turned off the xdebug lib is all the time installed.
Description
We need to add mapping to nginx configuration so it will pass the request to concrete phpfpm container based on the cookie attached to request.
Changes in the codebase
The changes includes separation of
phpfpm
container to two separate container (phpfpm
,phpfpm-xdebug
). This allows us to drop xdebug forphpfpm
container.We also have changed the socket names for each of the containers which are named accordingly
/sock/phpfpm.sock
and/sock/phpfpm-xdebug.sock
. Since we are running both in the same time there were conflicts regarding names so that what was needed to be done.There are also changes regarding nginx configuration regarding
app
container which uses this image. First - we adjusted upstream definition after changes of name for currently existing phpfpm one and added second one which covers new one regardingxdebug
.The magic and thing that does the job is the next one: map. Based on the cookie being sent within the request
XDEBUG_SESSION
we define to which phpfpm container the request should be passed.Additional information
The changes were tested within VSCode and PHPStorm configuration and we would need to adjust the README regarding the usage of the xdebug thing. For PHPStorm we need to declare the concrete name for record in
PHP | Servers
so it will map correctly to the files tree.This is a part of the changes that should be applied because the same thing should happen to CLI usage so instead of using whole FPM to run commands we could use smaller php CLI and php CLI with xdebug. The final outcome should give us: