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

refactor: merge Dockerfiles, fix XDebug crash, support PHP 8.3, disable XDebug by default #20

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

lucasmirloup
Copy link
Member

Note: Imagick is disabled for now in PHP 8.3 until Imagick/imagick#640 is fixed.

Resolves #16
Resolves #17
Resolves #18
Resolves #19

@lucasmirloup lucasmirloup added bug Something isn't working enhancement New feature or request labels Mar 6, 2024
@lucasmirloup lucasmirloup requested a review from thislg March 6, 2024 09:18
@lucasmirloup lucasmirloup self-assigned this Mar 6, 2024
Dockerfile Outdated
ARG PHP_VERSION
ARG PHP_EXTENSIONS

FROM php:${PHP_VERSION}-fpm
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: As recommended by hadolint (https://github.com/hadolint/hadolint) you should alias

Suggested change
FROM php:${PHP_VERSION}-fpm
FROM php:${PHP_VERSION}-fpm as base
FROM base

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it, but hadolint doesn't seem to require aliasing FROM in this case 🤔

On their rule list, the closest rules I can find that could apply to this case are DL3022 and DL3024, but they're not a problem here..

Dockerfile Outdated
Comment on lines 6 to 9
WORKDIR /usr/local/etc/php/conf.d/
COPY --link symfony.ini .
WORKDIR /usr/local/etc/php/pool.d/
COPY --link symfony.pool.conf .
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick (non-blocking): ‏You could use this instead IMO

Suggested change
WORKDIR /usr/local/etc/php/conf.d/
COPY --link symfony.ini .
WORKDIR /usr/local/etc/php/pool.d/
COPY --link symfony.pool.conf .
WORKDIR /usr/local/etc/php
COPY --link symfony.ini ./conf.d/
COPY --link symfony.pool.conf ./pool.d/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

Dockerfile Outdated
WORKDIR /usr/local/etc/php/pool.d/
COPY --link symfony.pool.conf .

ADD --chmod=0755 https://github.com/mlocati/docker-php-extension-installer/releases/latest/download/install-php-extensions /usr/local/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: ‏You should the script from the official docker image instead of downloading the latest

Suggested change
ADD --chmod=0755 https://github.com/mlocati/docker-php-extension-installer/releases/latest/download/install-php-extensions /usr/local/bin/
COPY --from=extension-installer /usr/bin/install-php-extensions /usr/local/bin

And add your dependencies at the top :

+ FROM mlocati/php-extension-installer:2 as extension-installer
  FROM php:${PHP_VERSION}-fpm as base

It is safer and you will have to update manually this dependency whenever a new major version is published

Copy link
Member Author

@lucasmirloup lucasmirloup Mar 6, 2024

Choose a reason for hiding this comment

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

I originally chose this approach from their documentation because of the warning about the Docker method, but as it is built in a CI, it should always pull the latest version of the image, so it should be fine to switch to FROM.

@aegypius
Copy link
Contributor

aegypius commented Mar 6, 2024

praise: ‏Great work !

Maybe you could add hadolint to the CI to spot some issues (or ignore them with a comment if absolutely necessary)

@aegypius
Copy link
Contributor

aegypius commented Mar 6, 2024

note (non-blocking): ‏I think you could drop support for 7.4 as mentionned in the README (supported until 14 Dec. 2023) or update the readme.

The 7.x line is no longer supported by PHP so the build can fail any moment now. I would probably isolate the 7.4 build to allow to push image even if php 7.4 image fails

@lucasmirloup
Copy link
Member Author

lucasmirloup commented Mar 6, 2024

note (non-blocking): ‏I think you could drop support for 7.4 as mentionned in the README (supported until 14 Dec. 2023) or update the readme.

The 7.x line is no longer supported by PHP so the build can fail any moment now. I would probably isolate the 7.4 build to allow to push image even if php 7.4 image fails

I updated the 7.4 and 8.0 images because they could be affected by the bug described in #19, and I wanted to be sure that the last images built for these versions are free from this kind of blocking bug.
I plan to build them only once and then remove them from the configuration 👍

@lucasmirloup lucasmirloup requested a review from aegypius March 6, 2024 13:21
@lucasmirloup lucasmirloup force-pushed the refactor/merge-dockerfiles branch from 4793965 to 4835cdf Compare March 8, 2024 10:32
@thislg thislg merged commit b855f58 into master Mar 11, 2024
2 checks passed
@thislg thislg deleted the refactor/merge-dockerfiles branch March 11, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
3 participants