Working on PHP 8.4 build#12
Conversation
WalkthroughThe changes introduce a new Makefile and Dockerfiles for PHP version 8.4, establishing build processes for both CLI and FPM environments based on Alpine Linux. The Makefile defines a build process with targets for creating and pushing Docker images. The Dockerfiles configure PHP environments with necessary extensions and settings, while a new entrypoint script manages container configuration for New Relic and Postfix, adapting settings based on environment variables. Additionally, the GitHub Actions workflow is updated to reflect a new minimum version requirement for periodic updates. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
src/8.4/cli-alpine/Dockerfile (5)
4-4: Consider using a stable XDebug release instead of a beta versionYou have specified
XDEBUG_RELEASE=3.4.0beta1, which is a beta version. For production environments, it's recommended to use stable releases to ensure reliability and avoid potential issues associated with beta versions.
45-45: Remove duplicatezlib-devfrom build dependenciesThe package
zlib-devis listed twice in the.build-depsvirtual package. This redundancy can be eliminated to simplify the build process.Apply this diff to remove the duplicate entry:
$PHPIZE_DEPS \ zlib-dev \ icu-dev \ g++ \ freetype-dev \ libpng-dev \ imagemagick-dev \ libjpeg-turbo-dev \ ghc \ autoconf \ krb5-dev \ libssh2-dev \ - zlib-dev \ patch \Also applies to: 56-56
52-52: Verify the necessity of installingghcThe
ghc(Glasgow Haskell Compiler) package is included in the.build-deps. Unless there's a specific requirement to compile Haskell code during the build process, consider removing it to reduce image size and build time.
106-106: Reconsider preloadinglibiconvusingLD_PRELOADUsing
LD_PRELOADto load/usr/lib/preloadable_libiconv.sois a workaround that may introduce unpredictable behavior. It's better to address the root cause by ensuring propericonvsupport within the base image or PHP configuration.Would you like assistance in finding an alternative solution to handle character encoding without preloading
libiconv?
19-32: Optimize package installation to reduce image sizePackages like
imagemagick,ghostscript,libssh2, andicuare installed in.php-deps. If they're only required for building extensions and not needed at runtime, consider moving them to.build-depsto minimize the final image size.src/8.4/fpm-alpine/Dockerfile (3)
119-119: Use caution when settingLD_PRELOADforlibiconvSetting
LD_PRELOADto/usr/lib/preloadable_libiconv.socan lead to unexpected behavior and potential conflicts within the container environment.Instead of using
LD_PRELOAD, consider installing a compatible version oflibiconvor ensuring that all dependencies are properly linked at build time. This avoids the need for runtime library overrides.If
libiconvis necessary, you may compile PHP with the required flags or use packages that do not require this workaround.
134-135: Properly disable Xdebug extensionDisabling Xdebug by commenting out the first line of its
.inifile may not fully prevent it from loading. This approach can be unreliable.A better practice is to remove the Xdebug configuration file or use the
phpdismodcommand to disable the extension.Apply this diff:
-&& sed -i "1s/^/;/" /usr/local/etc/php/conf.d/docker-php-ext-xdebug.ini \ +&& rm /usr/local/etc/php/conf.d/docker-php-ext-xdebug.ini \This ensures that Xdebug is entirely disabled. When needed, you can enable it by copying the configuration file back into the
conf.ddirectory.
53-53: Duplicate package installation:zlib-devThe package
zlib-devis listed twice in the build dependencies, which is unnecessary.Remove the duplicate entry to keep the Dockerfile clean.
Apply this diff:
&& apk add --no-cache --virtual .build-deps \ $PHPIZE_DEPS \ zlib-dev \ icu-dev \ g++ \ freetype-dev \ libpng-dev \ imagemagick-dev \ libjpeg-turbo-dev \ autoconf \ krb5-dev \ libssh2-dev \ - zlib-dev \ patch \ && \Also applies to: 64-64
src/8.4/fpm-alpine/hiqdev-docker-entrypoint (1)
27-27: Correct the typo in the comment: 'Diable' → 'Disable'There's a minor typo in the comment. It should be "Disable Opcache" instead of "Diable Opcache".
Apply this diff to fix the typo:
- # Diable Opcache + # Disable Opcache
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/8.4/Makefile(1 hunks)src/8.4/cli-alpine/Dockerfile(1 hunks)src/8.4/fpm-alpine/Dockerfile(1 hunks)src/8.4/fpm-alpine/hiqdev-docker-entrypoint(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/8.4/Makefile
🔇 Additional comments (3)
src/8.4/cli-alpine/Dockerfile (1)
88-89:
Fix potential syntax error in command chaining
The docker-php-ext-install opcache \ command ends with a backslash indicating continuation, but there's no command on the next line. This could lead to a syntax error during the build process. Ensure that command chaining is correctly structured.
Apply this diff to fix the command:
- docker-php-ext-install opcache \
+ docker-php-ext-install opcache && \Likely invalid or redundant comment.
src/8.4/fpm-alpine/Dockerfile (2)
20-20: Verify error_reporting directive is correctly set
The PHP_ERROR_REPORTING environment variable includes quotes, which might lead to unintended literal quotes in the php.ini configuration.
Ensure that the error_reporting directive receives the intended value without extra quotes.
Apply this diff to remove quotes from the environment variable and correctly set quotes in the configuration:
-ENV PHP_ERROR_REPORTING="E_ALL & ~E_DEPRECATED & ~E_STRICT & ~E_NOTICE"
+ENV PHP_ERROR_REPORTING=E_ALL & ~E_DEPRECATED & ~E_STRICT & ~E_NOTICEAnd adjust the echo command:
-echo "error_reporting = $PHP_ERROR_REPORTING"; \
+echo "error_reporting = \"$PHP_ERROR_REPORTING\""; \This change ensures that php.ini contains the correct error_reporting value.
Also applies to: 168-168
145-149: Verify sed commands correctly update PHP-FPM configuration
The sed commands are intended to update the PHP-FPM configuration in www.conf. Ensure that these commands correctly match and replace the desired lines.
Run the following script to confirm the configurations have been updated:
Expected output:
pm.max_children = 300
pm.start_servers = 50
pm.min_spare_servers = 20
pm.max_spare_servers = 50
Ensure that the output matches the expected values to confirm that the sed commands worked as intended.
✅ Verification successful
The sed commands for PHP-FPM configuration are correctly implemented
The verification of the sed commands in the Dockerfile shows that:
- The commands are properly formatted and use correct regex patterns to match and replace the configuration values
- The environment variables (
FPM_MAX_CHILDREN,FPM_START_SERVERS, etc.) are defined at the beginning of the Dockerfile - The modifications are part of a Docker build process where the base image (
php:8.4-fpm-alpine) provides the defaultwww.conf
The reason we couldn't verify the actual file content is that the changes happen during the Docker image build process, not in the repository itself. The sed commands will modify the configuration file that exists in the base PHP-FPM image.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that PHP-FPM settings have been applied.
# Check the updated values in www.conf
grep -E "^pm\.(max_children|max_spare_servers|min_spare_servers|start_servers)" /usr/local/etc/php-fpm.d/www.conf
Length of output: 182
Script:
#!/bin/bash
# Let's first check if the www.conf file exists in the repository
fd "www.conf"
# Let's also check the Dockerfile content to understand the context better
cat "src/8.4/fpm-alpine/Dockerfile"
Length of output: 6403
| # Sockets | ||
| && docker-php-ext-install sockets \ | ||
|
|
||
| && apk add gnu-libiconv="$GNU_LIBICONV_RELEASE" --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ --allow-untrusted \ |
There was a problem hiding this comment.
Avoid using the --allow-untrusted flag when installing packages
The use of --allow-untrusted bypasses package signature verification, posing a security risk. It's advisable to remove this flag to ensure all installed packages are verified.
Apply this diff to remove the --allow-untrusted flag:
- && apk add gnu-libiconv="$GNU_LIBICONV_RELEASE" --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ --allow-untrusted \
+ && apk add gnu-libiconv="$GNU_LIBICONV_RELEASE" --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| && apk add gnu-libiconv="$GNU_LIBICONV_RELEASE" --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ --allow-untrusted \ | |
| && apk add gnu-libiconv="$GNU_LIBICONV_RELEASE" --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ \ |
| FROM php:8.4-fpm-alpine | ||
|
|
||
| ENV POSTFIX_MAILNAME hiqdev.com | ||
| ENV XDEBUG_RELEASE=3.4.0beta1 |
There was a problem hiding this comment.
Avoid using beta versions of Xdebug in production
The Dockerfile sets XDEBUG_RELEASE to a beta version (3.4.0beta1). Using beta software in production environments can introduce instability and unexpected behavior.
Consider using the latest stable release of Xdebug. Update the XDEBUG_RELEASE environment variable accordingly.
Apply this diff:
-ENV XDEBUG_RELEASE=3.4.0beta1
+ENV XDEBUG_RELEASE=3.3.1 # Replace with the latest stable versionAlso applies to: 92-94
| libpng-dev \ | ||
| imagemagick-dev \ | ||
| libjpeg-turbo-dev \ | ||
| ghc \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unnecessary package installation: ghc
The ghc (Glasgow Haskell Compiler) package is being installed but is likely unnecessary for building PHP extensions or running PHP applications. Including it increases the image size significantly.
Consider removing ghc from the list of build dependencies to optimize the image size.
Apply this diff:
&& apk add --no-cache --virtual .build-deps \
$PHPIZE_DEPS \
zlib-dev \
icu-dev \
g++ \
freetype-dev \
libpng-dev \
imagemagick-dev \
libjpeg-turbo-dev \
- ghc \
autoconf \
krb5-dev \
libssh2-dev \
zlib-dev \
patch \
&& \Committable suggestion skipped: line range outside the PR's diff.
| && curl -L https://download.newrelic.com/php_agent/archive/$NEW_RELIC_AGENT_VERSION/newrelic-php5-$NEW_RELIC_AGENT_VERSION-linux-musl.tar.gz | tar -C /tmp -zx \ | ||
| && NR_INSTALL_USE_CP_NOT_LN=1 NR_INSTALL_SILENT=1 /tmp/newrelic-php5-$NEW_RELIC_AGENT_VERSION-linux-musl/newrelic-install install \ | ||
| && rm -rf /tmp/newrelic-php5-* /tmp/nrinstall* \ |
There was a problem hiding this comment.
Undefined environment variable: NEW_RELIC_AGENT_VERSION
The NEW_RELIC_AGENT_VERSION environment variable is used in the New Relic installation commands but is not defined in the Dockerfile. This will cause build failures due to an undefined variable.
To fix this issue, define the NEW_RELIC_AGENT_VERSION variable at the beginning of the Dockerfile with the appropriate version.
Apply this diff to add the missing environment variable:
+ENV NEW_RELIC_AGENT_VERSION=9.18.1.303 # Replace with the desired versionCommittable suggestion skipped: line range outside the PR's diff.
| # Sockets | ||
| && docker-php-ext-install sockets \ | ||
|
|
||
| && apk add gnu-libiconv=$GNU_LIBICONV_RELEASE --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ --allow-untrusted \ |
There was a problem hiding this comment.
Security risk: Using --allow-untrusted with apk add
The --allow-untrusted flag bypasses package signature verification, which exposes the system to potential security risks by installing unverified packages.
It's recommended to remove the --allow-untrusted flag and ensure packages are installed from trusted sources with proper signature verification.
Apply this diff:
-&& apk add gnu-libiconv=$GNU_LIBICONV_RELEASE --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ --allow-untrusted \
+&& apk add gnu-libiconv=$GNU_LIBICONV_RELEASE --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ \If necessary, add the appropriate repository keys or verify the package signatures to maintain security.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| && apk add gnu-libiconv=$GNU_LIBICONV_RELEASE --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ --allow-untrusted \ | |
| && apk add gnu-libiconv=$GNU_LIBICONV_RELEASE --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ \ |
| sed -i "s/xdebug.client_port\s*=\s*.*/xdebug.client_port=$XDEBUG_CLIENT_PORT/" /usr/local/etc/php/conf.d/docker-php-ext-xdebug.ini | ||
| fi | ||
| if [ -n "$XDEBUG_START_WITH_REQUEST" ]; then | ||
| echo "[ xdebug ] Set xdebug.remote_autostart to $XDEBUG_START_WITH_REQUEST" |
There was a problem hiding this comment.
Update the echo statement to match the actual XDebug parameter
The echo message refers to xdebug.remote_autostart, but the parameter being set is xdebug.start_with_request. To maintain consistency and avoid confusion, update the echo statement to reflect the correct parameter name.
Apply this diff to correct the echo statement:
- echo "[ xdebug ] Set xdebug.remote_autostart to $XDEBUG_START_WITH_REQUEST"
+ echo "[ xdebug ] Set xdebug.start_with_request to $XDEBUG_START_WITH_REQUEST"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "[ xdebug ] Set xdebug.remote_autostart to $XDEBUG_START_WITH_REQUEST" | |
| echo "[ xdebug ] Set xdebug.start_with_request to $XDEBUG_START_WITH_REQUEST" |
| if [ -n "$NEW_RELIC_LICENSE_KEY" ]; then | ||
| echo '[ New Relic ] Enabled' | ||
|
|
||
| sed -i 's/;extension/extension/' $(php -r "echo(PHP_CONFIG_FILE_SCAN_DIR);")/newrelic.ini |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine the sed command to target the specific New Relic extension line
The current sed command may unintentionally uncomment other lines starting with ;extension. To precisely uncomment the New Relic extension line, refine the pattern to match newrelic.so.
Apply this diff to make the sed command more precise:
- sed -i 's/;extension/extension/' $(php -r "echo(PHP_CONFIG_FILE_SCAN_DIR);")/newrelic.ini
+ sed -i 's/^;\s*extension\s*=\s*newrelic\.so/extension=newrelic.so/' $(php -r "echo(PHP_CONFIG_FILE_SCAN_DIR);")/newrelic.ini📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sed -i 's/;extension/extension/' $(php -r "echo(PHP_CONFIG_FILE_SCAN_DIR);")/newrelic.ini | |
| sed -i 's/^;\s*extension\s*=\s*newrelic\.so/extension=newrelic.so/' $(php -r "echo(PHP_CONFIG_FILE_SCAN_DIR);")/newrelic.ini |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/8.4/fpm-alpine/Dockerfile (1)
1-185: Overall assessment of the DockerfileThe Dockerfile provides a comprehensive PHP 8.4 FPM environment with good configuration practices, but there are several critical issues that need to be addressed:
- Using PHP 8.4 (development version) and Xdebug beta might cause stability issues
- Security concerns with
--allow-untrustedflag- Missing New Relic version definition
- Unnecessary
ghcpackage increasing image sizeOn the positive side:
- Well-structured configuration management
- Good security practice with XDebug disabled by default
- Comprehensive PHP extension setup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/main.yaml(1 hunks)src/8.4/cli-alpine/Dockerfile(1 hunks)src/8.4/fpm-alpine/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/8.4/cli-alpine/Dockerfile
🔇 Additional comments (7)
src/8.4/fpm-alpine/Dockerfile (7)
134-142: Good security practice: XDebug disabled by default
Excellent approach to disable XDebug by default and providing a comprehensive configuration that can be enabled when needed.
4-4:
Avoid using beta versions of Xdebug in production
The Dockerfile sets XDEBUG_RELEASE to a beta version (3.4.0beta1). Using beta software in production environments can introduce instability and unexpected behavior.
Apply this diff:
-ENV XDEBUG_RELEASE=3.4.0beta1
+ENV XDEBUG_RELEASE=3.3.1 # Use the latest stable version60-60: 🛠️ Refactor suggestion
Remove unnecessary ghc package
The Glasgow Haskell Compiler (ghc) package is not required for PHP builds and significantly increases the image size.
Apply this diff:
- ghc \108-108:
Security risk: Remove --allow-untrusted flag
Using --allow-untrusted with apk add bypasses package signature verification, exposing the system to potential security risks.
Apply this diff:
- && apk add gnu-libiconv=$GNU_LIBICONV_RELEASE --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ --allow-untrusted \
+ && apk add gnu-libiconv=$GNU_LIBICONV_RELEASE --update-cache --repository http://dl-cdn.alpinelinux.org/alpine/latest-stable/community/ \111-113:
Define NEW_RELIC_AGENT_VERSION environment variable
The New Relic installation commands use an undefined environment variable NEW_RELIC_AGENT_VERSION.
Add the following line near the other ENV declarations at the top of the Dockerfile:
+ENV NEW_RELIC_AGENT_VERSION=10.11.0.3183-183: Verify entrypoint script existence and permissions
Ensure that the hiqdev-docker-entrypoint script exists and has proper executable permissions.
✅ Verification successful
Entrypoint script exists and has correct permissions
The hiqdev-docker-entrypoint script exists in the correct location (src/8.4/fpm-alpine/) and has the proper executable permissions (-rwxr-xr-x), allowing it to be executed by all users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the entrypoint script exists and is executable
fd -t f "hiqdev-docker-entrypoint" -x ls -l {}
Length of output: 1145
1-1: Verify PHP 8.4 base image stability
PHP 8.4 is currently in development and not yet released for production use. This might lead to stability issues.
✅ Verification successful
PHP 8.4 is released and stable
Based on the PHP releases page output, PHP 8.4.1 has been officially released as of November 21, 2024, making it a stable production version. Using PHP 8.4 as the base image is appropriate and follows good practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check PHP 8.4 release status and available versions
curl -s https://www.php.net/releases/index.php | grep -A 5 "8.4"
Length of output: 59682
|
|
||
| env: | ||
| PERIODIC_UPDATES_MIN_VERSION: 7.4 | ||
| PERIODIC_UPDATES_MIN_VERSION: 8.3 |
There was a problem hiding this comment.
💡 Codebase verification
Setting PERIODIC_UPDATES_MIN_VERSION to 8.3 needs reconsideration
Based on the repository structure and configuration:
- The repository currently supports PHP versions from 7.0 to 8.4
MIN_SUPPORTED_VERSIONis set to 7.3, meaning versions 7.0-7.2 are excluded from all builds- Setting
PERIODIC_UPDATES_MIN_VERSIONto 8.3 would stop periodic security updates for a large number of actively maintained versions (7.3-8.2) - This seems overly restrictive given that PHP 7.4, 8.0, 8.1, and 8.2 are still widely used in production
Recommendations:
- Consider keeping
PERIODIC_UPDATES_MIN_VERSIONat a lower version (e.g., 7.4 or 8.0) to continue providing security updates for stable PHP versions - If the intent is to reduce maintenance burden, consider a more gradual approach by incrementing the minimum version in smaller steps
- Document the rationale for excluding versions 7.4-8.2 from periodic updates if this change is intentional
🔗 Analysis chain
Verify the impact on older PHP versions
Increasing PERIODIC_UPDATES_MIN_VERSION from 7.4 to 8.3 means that PHP versions 7.4 through 8.2 will no longer receive periodic updates. Please confirm if this is intentional, as it might affect users relying on security updates for these versions.
Also, since this PR is working on PHP 8.4 build, why set the minimum version to 8.3 instead of 8.4?
Let's check the generate-actions.sh script and version support policy:
Note: I'm generating additional scripts to gather more context about version support policy and the rationale behind the version changes, particularly focusing on documentation and configuration files.
Note: I'm trying to understand the full context of version support by checking the available PHP versions and related configuration changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the generate-actions.sh script to understand version handling
cat scripts/generate-actions.sh
# Look for version support policy in documentation
fd -t f -e md -e txt . | xargs rg -i "support|version|policy"
Length of output: 1252
Script:
#!/bin/bash
# Let's check the repository's documentation and configuration files for version support information
rg -i "version|support" README.md
# Check if there are any recent changes or discussions about version support
fd -t f -e yaml -e yml . | xargs rg -i "min.*version|support.*version"
# Look for any PHP version related configurations
fd -t f composer.json | xargs cat
# Check for any version-related discussions in PR description or issue
gh pr view --json title,body
Length of output: 1823
Script:
#!/bin/bash
# Let's check what PHP versions are currently in the repository
ls -1 src/
# Check the current MIN_SUPPORTED_VERSION in the workflow
rg "MIN_SUPPORTED_VERSION:" .github/workflows/
# Look for any recent commits or changes related to version support
git log --oneline -n 10 .github/workflows/main.yaml
Length of output: 156
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes