Conversation
WalkthroughAdds Microsoft ODBC driver installation and runtime copying; expands PHP extensions installed in the build stage to include many new extensions (pspell, pcntl, mongodb, ldap, imap, grpc, gmp, gettext, excimer, ev, calendar, xsl, sqlsrv, pdo_sqlsrv, pdo_pgsql) and updates runtime libraries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@php/Dockerfile`:
- Around line 37-40: The Dockerfile uses curl with the -k flag for the
msodbcsql18 and mssql-tools18 APKs (the download commands that fetch
msodbcsql18_18.6.1.1-1_${ARCH}.apk/.sig and
mssql-tools18_18.6.1.1-1_${ARCH}.apk/.sig); remove the -k option to re-enable
TLS certificate validation and replace with safer flags like --fail --show-error
--remote-name to fail on HTTP errors and surface issues, keeping the existing
GPG signature verification and ensuring ca-certificates is installed before
these curl commands.
- Around line 32-44: The Dockerfile currently installs msodbcsql18 and
mssql-tools only in the builder stage (the lines that download
msodbcsql18_18.6.1.1-1_${ARCH}.apk and mssql-tools18_18.6.1.1-1_${ARCH}.apk and
run apk add --allow-untrusted), so the final runtime image lacks the ODBC driver
and odbcinst.ini; update the Dockerfile to either move the entire driver install
block into the final stage or copy the installed artifacts
(msodbcsql18_18.6.1.1-1_${ARCH}.apk contents, mssql-tools files, and
odbcinst.ini) plus install the unixODBC runtime dependency into the final image
so sqlsrv / pdo_sqlsrv can find the driver at runtime.
|
@ma-04 where are we on this? |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
php/Dockerfile (1)
53-63:⚠️ Potential issue | 🔴 CriticalImagick source build for PHP 8.3 uses
[[ ]]which requires bash, but Alpine's default shell is/bin/sh.The
RUNinstruction uses[[ "$PHP_VERSION" == "8.3"* ]]which is a bash-ism. Docker'sRUNuses/bin/shby default on Alpine (which isash). This will fail with a syntax error unlessSHELLis overridden.🐛 Fix: use POSIX-compatible test or set SHELL
Option 1 — POSIX-compatible pattern match:
-RUN if [[ "$PHP_VERSION" == "8.3"* ]]; then \ +RUN if echo "$PHP_VERSION" | grep -q "^8\.3"; then \Option 2 — Use
case:-RUN if [[ "$PHP_VERSION" == "8.3"* ]]; then \ +RUN case "$PHP_VERSION" in 8.3*) \ git clone https://github.com/Imagick/imagick.git /usr/src/php/ext/imagick && \ ... - else \ - install-php-extensions imagick; \ - fi + ;; *) install-php-extensions imagick ;; esac
🤖 Fix all issues with AI agents
In `@php/Dockerfile`:
- Around line 84-88: The runtime image is missing several shared libraries
required by compiled PHP extensions and incorrectly includes the development
package libssh2-dev; update the RUN apk add line to remove libssh2-dev and add
the runtime libraries needed by extensions (e.g., libpq or postgresql-libs for
pdo_pgsql, libldap for ldap, c-client for imap, gmp for gmp, aspell-libs for
pspell, libxslt for xsl, and libintl/gettext for gettext) while keeping libssh2
(not libssh2-dev) and unixodbc present so extensions that link dynamically will
find their shared libraries at runtime.
🧹 Nitpick comments (2)
php/Dockerfile (2)
87-87:libssh2-devshould likely belibssh2in the runtime stage.Development headers (
-devpackages) are unnecessary in the final image and increase image size. The runtime only needs the shared library.♻️ Proposed fix
- libssh2-dev lz4 libavif libxpm lz4-libs unixodbc && \ + libssh2 lz4 libavif libxpm lz4-libs unixodbc && \
50-50: Verify the full extension list works across all target PHP versions.This is a long single-line list of ~45 extensions. Some extensions (e.g.,
jsonis built-in since PHP 8.0,hash/pcreare always compiled in) may cause warnings or errors depending on the PHP version specified byPHP_VERSION(default 7.4). Also,sqlsrv/pdo_sqlsrvrequire the ODBC driver installed in the previous step—ensure the installer can find it.Consider: if any single extension fails to install, the entire
RUNlayer fails. For extensions with complex or version-specific requirements, installing them in separateRUNsteps (or grouping by risk) could improve debuggability.
|
@tareq1988 vai, current status: all extensions are working without issues after adding runtime dependencies. Tested with docker image 
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
php/Dockerfile (1)
11-19:⚠️ Potential issue | 🟡 MinorWP-CLI package installation at build time requires GitHub authentication that is not provided.
The
wp package install wp-cli/doctor-command:@stable--allow-rootcommand at line 15 requires GitHub API access but no authentication token is available during the Docker build. This is indeed unrelated to the PHP extension installation in the builder stage, but it's a code issue in the Dockerfile itself, not merely an environment configuration problem. Either pass a GitHub token during build (--build-arg GITHUB_TOKEN=...) or remove the package installation from the build stage.
|
Test failing due to dependency on fix: ci build failures php containers |
2 participants
Resolves #25
Notes
Installing
sqlsrvandpdo_sqlsrvrequires Microsoft ODBC driver. Microsoft download repository also has issues with curl in regards to ca-certificates.Summary by CodeRabbit