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

Let Xdebug work without specific settings for different OSes + some slight improvement #27

Merged
merged 6 commits into from
May 26, 2022

Conversation

HarkuLi
Copy link
Contributor

@HarkuLi HarkuLi commented May 21, 2022

Thanks to this repository, I learnt a lot about the combination of nginx and php-fpm.
Here's something I think that could probably be improved when reading the configs.


What does this implement/fix? Explain your changes.

  • Let Xdebug work without specific settings for different OSes
  • Improve README
  • Fix typo in a comment
  • Add missing Makefile targets in .PHONY and reorder it
  • Remove unused fpm config: pm.process_idle_timeout
  • Use /bin/sh instead for shell commands in the Makefile

Does this close any currently open issues?

No.

Any relevant logs, error output, etc?

No.

Any other comments?

No.

Where has this been tested?

  1. Ubuntu 20.04.4 LTS + Docker 20.10.14
  2. macOS Big Sur 11.6.6 + Docker Desktop 4.8.2 (Docker 20.10.14)

Dockerfile Show resolved Hide resolved
@sherifabdlnaby
Copy link
Owner

@HarkuLi Hello, thanks for your PR! I like this enhancement, however I have a small tweak in mind.

I am thinking of keeping the Environment Variable reference in PHP Config, as well as its default value in Dockerfile. This makes the image decoupled from the docker-compose file. So, just in case you wish not to use the already existing docker-compose.yaml file, you won't need to change anything the Dockerfile to make it "configurable".

HarkuLi added 6 commits May 24, 2022 21:42
* Arg `RUNTIME_DEPS` has been removed from the top of Dockerfile.
* Fix the display of "Build Time Arguments" table.
* Correct the default value of `XDEBUG_VERSION`.
According to https://www.php.net/manual/en/install.fpm.configuration.php,
`pm.process_idle_timeout` is only used when `pm` is set to `ondemand`.
Bash shell isn't available in the alpine image.
Copy link
Contributor Author

@HarkuLi HarkuLi left a comment

Choose a reason for hiding this comment

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

I have rebased the latest version of main branch because of a conflict.
Additionally,

  1. I find that the display of Build Time Arguments table in the latest version README file is broken, so I also rearrange the table in this PR.
  2. Because bash shell isn't available in the alpine image, I replace it with sh for shell commands in Makefile.

Dockerfile Show resolved Hide resolved
@sherifabdlnaby sherifabdlnaby merged commit 86f1354 into sherifabdlnaby:main May 26, 2022
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.

2 participants