PHP 8.4 support #22
PHP 8.4 support #22
Conversation
WalkthroughPHP baseline raised to 8.4 (Docker + Composer). Pool class refactored to use constructor property promotion and nullable Connection parameters. Tests updated to arrow functions, explicit void return types, and added #[\Override] on setUp methods. No public API additions or control-flow changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (3)
src/Pools/Pool.php (1)
65-69: Constructor property promotion — looks correct; consider documenting positive-int.Promotion for
$nameand$sizeis clean, and switching initialization to$this->sizeis correct. Consider tightening the phpdoc for size to communicate domain constraints:
- Add
@param positive-int $sizeto the ctor docblock to reflect valid input.Suggested doc change (outside the selected lines):
/** - * @param int $size + * @param positive-int $size * @param callable(): TResource $init */tests/Pools/PoolTest.php (2)
18-20: Prefer protected visibility for setUp() when using #[\Override]PHPUnit\TestCase::setUp() is protected. While widening visibility to public is allowed, matching the parent’s visibility avoids surprises with tooling and future PHPUnit internals.
- public function setUp(): void + protected function setUp(): void
269-280: Ensure resources are always reclaimed even if assertions failWrap the assertion in a try/finally so connections are reclaimed if an assertion throws. This makes the helper safe and prevents test contamination.
- $allocate = function (int $amount, callable $assertion): void { + $allocate = function (int $amount, callable $assertion): void { $connections = []; for ($i = 0; $i < $amount; $i++) { $connections[] = $this->object->pop(); } - - $assertion(); - - foreach ($connections as $connection) { - $this->object->reclaim($connection); - } + try { + $assertion(); + } finally { + foreach ($connections as $connection) { + $this->object->reclaim($connection); + } + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
Dockerfile(1 hunks)composer.json(2 hunks)src/Pools/Pool.php(3 hunks)tests/Pools/ConnectionTest.php(3 hunks)tests/Pools/GroupTest.php(6 hunks)tests/Pools/PoolTest.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Pools/Pool.php (2)
src/Pools/Connection.php (4)
__construct(22-24)reclaim(84-91)Connection(10-105)destroy(97-104)src/Pools/Group.php (1)
reclaim(48-55)
tests/Pools/GroupTest.php (3)
tests/Pools/ConnectionTest.php (1)
Override(17-21)tests/Pools/PoolTest.php (1)
Override(18-22)src/Pools/Group.php (4)
Group(8-129)add(19-23)get(30-33)use(66-72)
tests/Pools/ConnectionTest.php (3)
tests/Pools/GroupTest.php (1)
Override(14-18)tests/Pools/PoolTest.php (1)
Override(18-22)src/Pools/Connection.php (3)
getPool(65-68)Connection(10-105)setPool(74-78)
tests/Pools/PoolTest.php (3)
tests/Pools/ConnectionTest.php (1)
Override(17-21)tests/Pools/GroupTest.php (1)
Override(14-18)src/Pools/Pool.php (3)
Pool(14-369)use(192-205)count(295-298)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (15)
composer.json (3)
27-27: Raising minimum PHP to 8.4 — aligns with the PR goal.This keeps the library consistent with the codebase’s 8.4-only features (e.g., explicit nullable params). No issues on the constraint itself.
27-27: Plan the release as a breaking platform bump.Increasing the minimum PHP from 8.3 to 8.4 will exclude existing consumers on 8.3. Ensure this ships under the next appropriate release line (often a major), and call it out in the changelog and README.
41-43: Composer platform pinned to 8.4 — good for reproducible resolves.This prevents accidental installs of deps that don’t actually support 8.4 at runtime. Looks good.
tests/Pools/GroupTest.php (3)
14-16: Using #[\Override] on setUp() — good modernization.This guards against accidental signature drift with PHPUnit’s TestCase.
22-22: Arrow functions for pool factories simplify the tests.Cleaner and equivalent semantics for the init callables. All usages look correct.
Also applies to: 29-29, 40-40, 53-53, 70-70, 81-81
105-112: Explicit void return on test closure is a nice touch.Prevents accidental return value assertions and clarifies intent.
src/Pools/Pool.php (3)
304-316: Make parameter explicitly nullable for PHP 8.4.Changing
reclaim(?Connection $connection = null)removes the implicit-null default that 8.4 disallows. Matches usages from Group::reclaim() and Connection::reclaim().
322-340: Same explicit-null fix for destroy().
destroy(?Connection $connection = null)is the correct 8.4-safe signature and remains backward-compatible with existing call sites.
300-306: No implicit-null parameters found; ready for PHP 8.4 upgradeOur repository-wide search for any non-nullable type-hinted parameters defaulting to
nullreturned no matches, confirming there are no remaining signatures likeFoo $x = nullwithout a leading?. You can proceed with confidence that implicit-null parameters have been eliminated.tests/Pools/ConnectionTest.php (2)
17-21: #[\Override] on setUp() — good.Ensures correct override of PHPUnit lifecycle method with v11.
57-57: Arrow functions for Pool init callables — concise and clear.No behavior change; improves readability in tests.
Also applies to: 65-65, 82-82
tests/Pools/PoolTest.php (4)
21-21: Nice use of arrow function for the Pool initializerConcise and clear; fits the simple, side‑effect‑free factory.
114-118: Explicit void on callback is goodThe explicit : void improves static analysis and documents intent; aligns with Pool::use not requiring a return here.
284-288: Telemetry assertions: explicit void closures look goodTyped closures keep the helper’s contract tight and improve readability.
292-296: Second telemetry block is consistent and clearConsistent typing and expectations; reads well with the helper.
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 (2)
Dockerfile (2)
21-23: Missing build deps for Swoole HTTP/2 and MongoDB: add nghttp2-dev, openssl-dev, zlib-dev (and friends).You configure Swoole with
--enable-http2, but the image doesn’t installnghttp2-dev(required) oropenssl-dev(often required alongside HTTP/2). The MongoDB driver (bundled libmongoc/libbson) typically needsopenssl-devandzlib-devat build time. Without these, builds can fail or silently disable features.Recommend expanding the build deps:
- && apk add --no-cache postgresql-libs postgresql-dev make automake autoconf gcc g++ git brotli-dev \ + && apk add --no-cache \ + postgresql-libs postgresql-dev \ + make automake autoconf gcc g++ git \ + brotli-dev nghttp2-dev openssl-dev zlib-dev c-ares-dev \Note: c-ares is optional but commonly enabled by libmongoc; include if you want async DNS support.
35-41: Align Swoole configure flags with installed deps.If HTTP/2 and TLS are desired, explicitly enable both after adding the corresponding -dev packages. Otherwise, drop
--enable-http2to avoid a fragile build.Suggested configure:
- && phpize \ - && ./configure --enable-http2 \ + && phpize \ + && ./configure --enable-openssl --enable-http2 \ && make && make installFollow-up: In CI, validate with
php --ri swoole | grep -E 'HTTP2|SSL'.
♻️ Duplicate comments (1)
Dockerfile (1)
69-72: Hard-coded PHP extension ABI path will break on PHP 8.4 (use dynamic extension_dir).The COPY paths still target the PHP 8.3 ABI dir
no-debug-non-zts-20230831. On 8.4, the ABI suffix changes, so these copies will fail or place .so files in the wrong directory. Copy with a wildcard, then move into the runtimeextension_dir.Apply this diff:
@@ -COPY --from=composer /usr/local/src/vendor /usr/src/code/vendor -COPY --from=swoole /usr/local/lib/php/extensions/no-debug-non-zts-20230831/swoole.so /usr/local/lib/php/extensions/no-debug-non-zts-20230831/ -COPY --from=redis /usr/local/lib/php/extensions/no-debug-non-zts-20230831/redis.so /usr/local/lib/php/extensions/no-debug-non-zts-20230831/ -COPY --from=mongodb /usr/local/lib/php/extensions/no-debug-non-zts-20230831/mongodb.so /usr/local/lib/php/extensions/no-debug-non-zts-20230831/ +COPY --from=composer /usr/local/src/vendor /usr/src/code/vendor +# Copy built extensions from their builder stages regardless of ABI dir +COPY --from=swoole /usr/local/lib/php/extensions/*/swoole.so /usr/local/lib/php/extensions/ +COPY --from=redis /usr/local/lib/php/extensions/*/redis.so /usr/local/lib/php/extensions/ +COPY --from=mongodb /usr/local/lib/php/extensions/*/mongodb.so /usr/local/lib/php/extensions/ +# Move them into the actual extension_dir of this image +RUN ext_dir="$(php -r 'echo ini_get(\"extension_dir\");')" \ + && mkdir -p "$ext_dir" \ + && mv /usr/local/lib/php/extensions/swoole.so "$ext_dir/swoole.so" \ + && mv /usr/local/lib/php/extensions/redis.so "$ext_dir/redis.so" \ + && mv /usr/local/lib/php/extensions/mongodb.so "$ext_dir/mongodb.so"
🧹 Nitpick comments (2)
Dockerfile (2)
8-9: Composer stage: avoid blanket--ignore-platform-reqs; bump composer image.Using
--ignore-platform-reqscan mask incompatibilities with PHP 8.4 and extensions, yielding a vendor tree that differs from production. Prefer a recent Composer image and honoring the platform set in composer.json.Suggested changes:
-FROM composer:2.0 AS composer +FROM composer:2 AS composer @@ -RUN composer install --ignore-platform-reqs --optimize-autoloader \ - --no-plugins --no-scripts --prefer-dist +RUN composer install --optimize-autoloader --no-plugins --no-scripts --prefer-distIf your composer.json sets
"platform": {"php": "8.4.x"}, this will resolve deps against 8.4 without ignoring platform requirements.
63-68: Micro: collapse php.ini tweaks into one layer.Functionally fine, but you can reduce layers and keep the file tidy.
-RUN mv "$PHP_INI_DIR/php.ini-production" "$PHP_INI_DIR/php.ini" - -RUN echo "opcache.enable_cli=1" >> $PHP_INI_DIR/php.ini - -RUN echo "memory_limit=1024M" >> $PHP_INI_DIR/php.ini +RUN mv "$PHP_INI_DIR/php.ini-production" "$PHP_INI_DIR/php.ini" \ + && { \ + echo "opcache.enable_cli=1"; \ + echo "memory_limit=1024M"; \ + } >> "$PHP_INI_DIR/php.ini"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (2)
Dockerfile (2)
59-61: Order OK; ensure .so placement happens before runtime.Enabling via INI is fine even if the .so is copied later in the Dockerfile, because the module is loaded at container runtime. Just make sure the .so relocation to
extension_dir(see earlier comment) is in place before the container starts.After implementing the dynamic
extension_dirmove, runphp -min the final image to confirm the modules are found.
11-16: Please verify extension loading inside your PHP 8.4-alpine imageIt looks like the
php -mcheck ran on your host’s PHP 8.2 installation rather than in the newly builtphp:8.4-cli-alpinecontainer—so naturally none of the 8.4-specific extensions were present. To confirm that Redis, Swoole and MongoDB actually compile and load under PHP 8.4, please rebuild and run the image, then re-execute the check:# 1. Build the Docker image locally (from the directory containing your Dockerfile) docker build -t php-ext-test . # 2. Run the module-loading tests inside the container docker run --rm php-ext-test sh -c ' php -v && \ php -m | grep -E "redis|swoole|mongodb" || (echo "Extensions not loaded" && exit 1) && \ php --ri swoole | sed -n "1,120p" && \ php --ri redis | sed -n "1,80p" && \ php --ri mongodb | sed -n "1,120p" 'Expected outcome:
php -vreports PHP 8.4.xphp -mlistsredis,swoole, andmongodbphp --ri swooleshows HTTP/2 support if configuredphp --ri redisandphp --ri mongodboutput version and build details
Summary by CodeRabbit