Develop #58
Develop #58
Conversation
| Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes modernize the Brotli PHP extension by replacing legacy resource-based state management for incremental compression and decompression with dedicated PHP objects. New final classes, Changes
Poem
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:
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 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: 1
🧹 Nitpick comments (4)
README.md (1)
290-297: Rename$resourcein the examples for clarity.
The incremental‑API now returns real objects, yet the examples keep the legacy variable name$resourcewhich can confuse users skimming the docs.-$resource = brotli_compress_init(); +$ctx = brotli_compress_init(); ... -$compressed .= brotli_compress_add($resource, 'Hello, ', BROTLI_FLUSH); -$compressed .= brotli_compress_add($resource, 'World!', BROTLI_FLUSH); -$compressed .= brotli_compress_add($resource, '', BROTLI_FINISH); +$compressed .= brotli_compress_add($ctx, 'Hello, ', BROTLI_FLUSH); +$compressed .= brotli_compress_add($ctx, 'World!', BROTLI_FLUSH); +$compressed .= brotli_compress_add($ctx, '', BROTLI_FINISH);Same for the decompression example a few lines below.
brotli.stub.php (1)
60-70: Optional: provide concrete constant values in stubs for IDE support.
UNKNOWNmarkers work, but most IDEs (PHPStorm, Psalm, etc.) can offer better autocompletion / static analysis when the real literal is present. Consider exporting the numeric values viagenerate_stubs.phpor a small C helper.brotli.c (2)
1284-1289: Pointer‑sign warning – cast toconst uint8_t*.
ctx->next_inis declared asconst uint8_t*, butin_bufischar*.
On several compilers this produces a strict‑aliasing / pointer‑sign warning.- ctx->next_in = in_buf; + ctx->next_in = (const uint8_t *)in_buf;
1417-1420: Secondary unused variable (res) inbrotli_uncompress_add.
Same as above – safe to drop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(4 hunks)brotli.c(8 hunks)brotli.stub.php(1 hunks)php_brotli.h(1 hunks)
🔇 Additional comments (2)
php_brotli.h (1)
43-44:zend_object std;looks good – but double‑check ABI compatibility.
Placingzend_objectas the last field is the expected layout for “struct -with‑object” patterns and is consistent with theXtOffsetOf()usage inbrotli.c. Just make sure no C code still copies the struct by value (it would silently drop the trailingstdbytes).brotli.c (1)
1246-1249: Remove unused variableres.
The variable is never referenced and triggers -Wunused‑parameter warnings.- zval *res;[ suggest_nitpick ]
brotli.c Outdated
| #if PHP_VERSION_ID >= 80300 | ||
| static zend_object * | ||
| php_brotli_context_create_object(zend_class_entry *class_type) | ||
| { | ||
| php_brotli_context *intern = zend_object_alloc(sizeof(php_brotli_context), | ||
| class_type); | ||
| zend_object_std_init(&intern->std, class_type); | ||
| object_properties_init(&intern->std, class_type); | ||
| return &intern->std; | ||
| } |
There was a problem hiding this comment.
Missing std.handlers assignment → objects leak / wrong destructor.
For PHP ≥ 8.3 the custom creator never assigns the object handlers.
As a result the object keeps the default handlers, and free_obj (which
releases the Brotli state) is never called, causing encoder/decoder
instances to leak.
zend_object_std_init(&intern->std, class_type); object_properties_init(&intern->std, class_type); +/* hook up our custom handlers so dtor / comparisons work */ +intern->std.handlers = class_type->default_object_handlers + ? class_type->default_object_handlers + : &std_object_handlers; return &intern->std;There was a problem hiding this comment.
🧩 Analysis chain
@kjdev I've reviewed the code regarding the potential issue with object handlers in PHP ≥ 8.3 that was previously identified.
I'm happy to report that the current implementation properly handles object creation and destruction for all PHP versions. The specific issue mentioned in the previous review (missing std.handlers assignment in the PHP 8.3-specific implementation) doesn't appear to be present in the code.
The current implementation:
- Has a generic
php_brotli_context_create_objectfunction that correctly takes handlers as a parameter and assigns them - Both specialized context creation functions (
php_brotli_compress_context_create_objectandphp_brotli_uncompress_context_create_object) correctly pass their handlers to this function - The handlers are properly initialized with the correct
free_objfunction that will clean up the Brotli state
No changes are needed as the code already handles this correctly.
🏁 Script executed:
#!/bin/bash # Check if the issue is still present in the PR branch grep -A 20 "php_brotli_context_create_object" brotli.c Length of output: 2139
🏁 Script executed:
#!/bin/bash # Check the compress and uncompress object creation functions grep -n -A 20 "php_brotli_compress_context_create_object" brotli.c grep -n -A 20 "php_brotli_uncompress_context_create_object" brotli.c Length of output: 5395
🏁 Script executed:
#!/bin/bash # Get the full context of the php_brotli_context_create_object function grep -n -A 40 "php_brotli_context_create_object" brotli.c Length of output: 3991
🏁 Script executed:
#!/bin/bash # Check the initialization of handlers grep -n -A 10 "php_brotli_compress_context_object_handlers" brotli.c grep -n -A 10 "php_brotli_uncompress_context_object_handlers" brotli.c Length of output: 4041
🏁 Script executed:
#!/bin/bash # Find where the handlers are initialized grep -n -A 30 "memcpy.*php_brotli_compress_context_object_handlers" brotli.c Length of output: 1632
🏁 Script executed:
#!/bin/bash # Find where the uncompress handlers are initialized grep -n -A 30 "memcpy.*php_brotli_uncompress_context_object_handlers" brotli.c Length of output: 1313
🏁 Script executed:
#!/bin/bash # Check for PHP 8.3-specific implementations grep -n -B 5 -A 15 "PHP_VERSION_ID >= 80300" brotli.c Length of output: 2186
🏁 Script executed:
#!/bin/bash # Check the MINIT function for PHP 8.3-specific code grep -n -A 100 "ZEND_MINIT_FUNCTION" brotli.c | grep -B 20 -A 20 "80300" Length of output: 3605
🏁 Script executed:
#!/bin/bash # Do a more thorough check for PHP 8.3 specific code grep -n "PHP_VERSION_ID >= 80" brotli.c Length of output: 627
@kjdev I've reviewed the code regarding the object handlers in PHP ≥ 8.3 that was previously identified as an issue.
I'm happy to report that the current implementation correctly handles object creation and destruction for all PHP versions. The specific issue mentioned in the previous review isn't present in the code.
The current implementation is well-structured:
- The
php_brotli_context_create_objectfunction correctly accepts handlers as a parameter and assigns them tointern->std.handlers - Both specialized creation functions properly pass their respective handlers
- The handlers are correctly initialized with
free_obj = php_brotli_context_free_obj - For PHP 8.3+, there's also optimization code that sets
default_object_handlerson the class
Everything looks good! No changes are needed as the code already handles object lifecycle correctly.
🧬 Code Graph Analysis Results
Relevant snippets from brotli.stub.php (lines 75-85)
function compress(string $data, int $quality = \BROTLI_COMPRESS_LEVEL_DEFAULT, int $mode = \BROTLI_GENERIC): string|false {} function uncompress(string $data, int $length = 0): string|false {} function compress_init(int $quality = \BROTLI_COMPRESS_LEVEL_DEFAULT, int $mode = \BROTLI_GENERIC): Compress\Context|false {} function compress_add(Compress\Context $context, string $data, int $mode = \BROTLI_FLUSH): string|false {} function uncompress_init(): UnCompress\Context|false {} function uncompress_add(UnCompress\Context $context, string $data, int $mode = \BROTLI_FLUSH): string|false {}These function declarations correspond directly to the ZEND_FUNCTION implementations in brotli.c for the Brotli compression and decompression API, including incremental compression/uncompression contexts and operations.
Summary of key relevant parts in brotli.c
-
Functions implementing Brotli compression and decompression:
-
brotli_compress(lines ~400-440): Compresses a string with optional quality and mode parameters, returns compressed string or false on failure. -
brotli_uncompress(lines ~520-560): Decompresses a Brotli compressed string with optional max output size, returns decompressed string or false on failure. -
brotli_compress_init(lines ~445-460): Initializes an incremental compression context object, returns the context or false on failure. -
brotli_compress_add(lines ~465-520): Adds data to an incremental compression context, with optional mode (flush/finish), returns compressed chunk or false on failure. -
brotli_uncompress_init(lines ~565-575): Initializes an incremental decompression context object, returns the context or false on failure. -
brotli_uncompress_add(lines ~580-630): Adds data to an incremental decompression context, with optional mode, returns decompressed chunk or false on failure.
-
-
Context object management:
-
php_brotli_contextstruct and related functions for creating, freeing, and managing Brotli compression and decompression contexts. -
Classes
Brotli\Compress\ContextandBrotli\UnCompress\Contextare registered internally and cannot be directly instantiated by user code; instead, the init functions must be used.
-
-
Stream wrapper for
compress.brotli://(lines ~350-390): Implements PHP stream wrapper for Brotli compression and decompression, supporting reading and writing streams with Brotli encoding. -
Output buffering handler (lines ~130-210): Implements an output handler for Brotli compression of PHP output buffers, including initialization, compression streaming, and cleanup.
-
Module initialization and configuration (lines ~270-330): Registers constants, classes, INI entries, output handler aliases, and stream wrappers.
-
APCu serializer support (lines ~640-700): Optional support for serializing and unserializing data compressed with Brotli for APCu caching.
This information provides a comprehensive overview of the Brotli PHP extension's core functionality as implemented in brotli.c, matching the function signatures declared in the stub file.
…li\Compress\Context class
…orli\UnCompress\Context class
Summary by CodeRabbit
New Features
Brotli\Compress\ContextandBrotli\UnCompress\Context) instead of generic resources.Documentation
Refactor