Develop #48
Develop #48
Conversation
WalkthroughThe recent updates involve enhancing the Brotli compression functionality by modifying function signatures, introducing new constants, and adjusting the compression and decompression logic. Test scripts have been adapted to cover these enhancements, with a focus on iterating through various modes, improving error handling, and generating warnings for invalid inputs. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- brotli.c (4 hunks)
- tests/compress_add.phpt (1 hunks)
- tests/incremental_compress_add_alias.phpt (1 hunks)
- tests/incremental_compress_add_args.phpt (2 hunks)
- tests/incremental_compress_add_basic.phpt (1 hunks)
- tests/incremental_uncompress_add_alias.phpt (1 hunks)
- tests/incremental_uncompress_add_basic.phpt (1 hunks)
Additional comments: 14
tests/incremental_compress_add_basic.phpt (2)
- 10-13: The introduction of
$modeTypesarray withBROTLI_PROCESSandBROTLI_FLUSHis a good addition for testing different modes. However, ensure that these constants are defined and have the correct values as expected.- 15-28: The refactored loop to iterate over
$modeTypesand testbrotli_compress_addwith different modes is a logical change. However, it's important to ensure thatbrotli_compress_initandbrotli_compress_addfunctions handle these modes correctly and that theBROTLI_FINISHoperation is always called after the loop to properly finalize the compression.tests/incremental_compress_add_alias.phpt (2)
- 10-13: The
$modeTypesarray is correctly defined withBROTLI_PROCESSandBROTLI_FLUSH. As with the previous file, ensure these constants are correctly defined.- 15-26: The loop over
$modeTypesto test the namespacedBrotli\compress_addfunction with different modes is consistent with the changes in the previous file. Again, verify that theBrotli\compress_initandBrotli\compress_addfunctions are implemented to support these modes and thatBROTLI_FINISHis used correctly.tests/incremental_uncompress_add_basic.phpt (2)
- 10-13: The
$modeTypesarray is introduced here as well, which is consistent with the other test files. Verify the definition of these constants.- 15-34: The addition of a loop to test
brotli_uncompress_addwith different modes is a good approach to ensure that the function works as expected across various scenarios. Make sure that thebrotli_uncompress_initandbrotli_uncompress_addfunctions are properly implemented to handle these modes.tests/incremental_uncompress_add_alias.phpt (2)
- 10-13: The
$modeTypesarray is consistently used across all test files, which is good for maintainability. Verify the constants' definitions.- 15-34: The changes are consistent with the non-namespaced version of the test, iterating over different modes for the
Brotli\uncompress_addfunction. Ensure that the namespacedBrotli\uncompress_initandBrotli\uncompress_addfunctions are correctly implemented to support these modes.tests/compress_add.phpt (1)
- 11-37: No changes are annotated in this file. If there were supposed to be changes related to the PR objectives, such as testing different modes, they are not present. If this is intentional and no changes are required for this test, then it's fine as is.
tests/incremental_compress_add_args.phpt (3)
- 13-16: The
$modeTypesarray is added to test different modes, which is consistent with the other test files. Verify the constants' definitions.- 18-33: The loop over
$modeTypesto test thebrotli_compress_addfunction with different modes is a logical change. Ensure that thebrotli_compress_initandbrotli_compress_addfunctions are implemented to support these modes and thatBROTLI_FINISHis used correctly.- 48-89: The warnings for invalid compression levels and modes are a good addition for robustness. Ensure that these warnings are correctly triggered and that the test reflects the expected behavior.
brotli.c (2)
- 983-989: The constants
BROTLI_FLUSH,BROTLI_FINISH, andBROTLI_EMIT_METADATAhave been added to the extension. These constants are used to control the compression process.- 980-992: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1443-1509]
The APC serializer and unserializer functions for Brotli have been added. These functions are responsible for serializing and unserializing PHP variables with Brotli compression. Ensure that the APCu extension is compatible with these functions and that they are correctly registered.
Verification successful
The search for APCu source files using
fddid not yield any results, which suggests that the APCu source code may not be present in the repository or not named in a way that matches the pattern provided.However, the
rgcommand successfully found a reference toapc_register_serializerin thebrotli.cfile. The context provided around this match indicates that the Brotli serializer and unserializer functions are indeed registered within the APCu extension, but only if the PHP version is 7 or higher and APCu support is enabled (HAVE_APCU_SUPPORTis defined).Given this information, we can confirm that the Brotli serializer and unserializer functions are intended to be registered with APCu for PHP 7 and above, as per the conditions checked in the preprocessor directives.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if APCu is enabled and if the Brotli serializer is correctly registered. php -m | grep apcu php -i | grep 'APC Serializer'Length of output: 195
Script:
#!/bin/bash # Attempt to find the APCu extension source code to check for Brotli serializer registration. fd 'apcu.*\.(c|h|php)$'Length of output: 25
Script:
#!/bin/bash # Search for the registration of Brotli serializer functions in the codebase. rg 'apc_register_serializer' --context 5Length of output: 519
| const uint8_t *next_in = in_buf; | ||
| size_t available_in = in_size; | ||
|
|
||
| if (in_size > 0) { | ||
| while (available_in) { | ||
| size_t available_out = buffer_size; | ||
| uint8_t *next_out = buffer; | ||
| if (BrotliEncoderCompressStream(ctx->encoder, | ||
| mode, | ||
| &available_in, | ||
| &next_in, | ||
| &available_out, | ||
| &next_out, | ||
| 0)) { | ||
| buffer_used = (size_t)(next_out - buffer); | ||
| if (buffer_used) { | ||
| smart_string_appendl(&out, buffer, buffer_used); | ||
| } | ||
| } else { | ||
| efree(buffer); | ||
| smart_string_free(&out); | ||
| php_error_docref(NULL TSRMLS_CC, E_WARNING, | ||
| "Brotli incremental compress failed\n"); | ||
| RETURN_FALSE; | ||
| while (available_in || BrotliEncoderHasMoreOutput(ctx->encoder)) { | ||
| size_t available_out = buffer_size; | ||
| uint8_t *next_out = buffer; | ||
| if (BrotliEncoderCompressStream(ctx->encoder, | ||
| mode, | ||
| &available_in, | ||
| &next_in, | ||
| &available_out, | ||
| &next_out, | ||
| 0)) { | ||
| buffer_used = (size_t)(next_out - buffer); | ||
| if (buffer_used) { | ||
| smart_string_appendl(&out, buffer, buffer_used); | ||
| } | ||
| } else { | ||
| efree(buffer); | ||
| smart_string_free(&out); | ||
| php_error_docref(NULL TSRMLS_CC, E_WARNING, | ||
| "Brotli incremental compress failed\n"); | ||
| RETURN_FALSE; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The search for brotli_compress_add usage within PHP files revealed a definition in brotli.stub.php with a default mode of BROTLI_PROCESS. This suggests that the default mode was previously BROTLI_PROCESS, and the change to BROTLI_OPERATION_FLUSH in the actual implementation is not reflected in the stub file. This discrepancy could cause inconsistencies in developer expectations and static analysis results.
Analysis chain
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1181-1224]
The brotli_compress_add function has been updated to use BROTLI_OPERATION_FLUSH as the default mode. This change affects how the compression stream is processed. Ensure that all calls to this function are reviewed to accommodate this change in behavior.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of brotli_compress_add to ensure compatibility with the new default mode.
rg --type php 'brotli_compress_add'
Length of output: 151
| zval *res; | ||
| php_brotli_state_context *ctx; | ||
| size_t buffer_size; | ||
| zend_long mode = BROTLI_OPERATION_PROCESS; | ||
| zend_long mode = BROTLI_OPERATION_FLUSH; | ||
| char *in_buf; | ||
| size_t in_size; | ||
| smart_string out = {0}; |
There was a problem hiding this comment.
Tip
Codebase Verification
The usage of brotli_uncompress_add found in brotli.stub.php indicates that the default value for the mode parameter is BROTLI_PROCESS. This is inconsistent with the changes in brotli.c, where the default value is set to BROTLI_OPERATION_FLUSH. The stub file should be updated to reflect the actual implementation to ensure consistency and correct static analysis.
Analysis chain
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1362-1392]
The brotli_uncompress_add function has been updated, and it now accepts a mode parameter. This change could affect how decompression is handled. Verify that all calls to this function are compatible with the new parameter.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of brotli_uncompress_add to ensure compatibility with the new parameter.
rg --type php 'brotli_uncompress_add'
Length of output: 155
|
@coderabbitai review |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- README.md (6 hunks)
- brotli.stub.php (4 hunks)
Additional comments: 12
brotli.stub.php (4)
- 17-17: The default value for
$modeinbrotli_compress_addhas been changed toBROTLI_FLUSH. Ensure that this change is reflected in all parts of the codebase where the function is used.- 27-27: The default value for
$modeinbrotli_uncompress_addhas been changed toBROTLI_FLUSH. Verify that this change is compatible with all usages of the function.- 44-44: The default value for
$modein the namespacedcompress_addfunction has been updated to\BROTLI_FLUSH. Confirm that this change does not affect existing functionality.- 54-54: The default value for
$modein the namespaceduncompress_addfunction has been updated to\BROTLI_FLUSH. This change should be checked for compatibility with current usages.README.md (8)
- 165-165: The documentation for
brotli_compress_addhas been updated to reflect the new default mode parameter. Ensure that the documentation is consistent with the actual function signature and behavior.- 181-181: The mode parameter description for
brotli_compress_addnow includesBROTLI_FLUSHas the default. Verify that the documentation accurately describes the behavior of the function with this default mode.- 208-208: The documentation for
brotli_uncompress_addhas been updated to reflect the new default mode parameter. Confirm that the documentation matches the actual function signature and behavior.- 224-224: The mode parameter description for
brotli_uncompress_addnow includesBROTLI_FLUSHas the default. Ensure that the documentation correctly describes the function's behavior with this default mode.- 240-240: The namespaced
compress_addfunction documentation has been updated to reflect the new default mode parameter. Check that the documentation is consistent with the actual function signature and behavior.- 242-242: The namespaced
uncompress_addfunction documentation has been updated to reflect the new default mode parameter. Ensure that the documentation is consistent with the actual function signature and behavior.- 296-297: The example code for incremental compression has been updated to use
BROTLI_FLUSH. Confirm that the example code works as intended with the new default mode.- 305-306: The example code for incremental uncompression has been updated to use
BROTLI_FLUSH. Verify that the example code functions correctly with the new default mode.
|
@coderabbitai review |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (22)
- README.md (6 hunks)
- brotli.c (10 hunks)
- brotli.stub.php (4 hunks)
- tests/alias.phpt (1 hunks)
- tests/apcu_serializer.phpt (1 hunks)
- tests/compress_add.phpt (1 hunks)
- tests/incremental_compress_add_alias.phpt (1 hunks)
- tests/incremental_compress_add_args.phpt (2 hunks)
- tests/incremental_compress_add_basic.phpt (1 hunks)
- tests/incremental_uncompress_add_alias.phpt (1 hunks)
- tests/incremental_uncompress_add_basic.phpt (1 hunks)
- tests/ob_001.phpt (1 hunks)
- tests/ob_002.phpt (1 hunks)
- tests/ob_003.phpt (1 hunks)
- tests/ob_004.phpt (1 hunks)
- tests/ob_005.phpt (1 hunks)
- tests/ob_006.phpt (1 hunks)
- tests/ob_007.phpt (1 hunks)
- tests/ob_009.phpt (1 hunks)
- tests/ob_010.phpt (1 hunks)
- tests/streams_005.phpt (1 hunks)
- tests/streams_006.phpt (1 hunks)
Files skipped from review due to trivial changes (11)
- tests/apcu_serializer.phpt
- tests/ob_001.phpt
- tests/ob_002.phpt
- tests/ob_004.phpt
- tests/ob_005.phpt
- tests/ob_006.phpt
- tests/ob_007.phpt
- tests/ob_009.phpt
- tests/ob_010.phpt
- tests/streams_005.phpt
- tests/streams_006.phpt
Additional comments: 26
tests/ob_003.phpt (1)
- 4-9: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the PHP version check condition might introduce compatibility issues with older PHP versions. Ensure that all functionalities used in the test are compatible with PHP versions older than 5.4.0.
tests/alias.phpt (1)
- 1-1: The removal of the
SKIPIFsection and its associated PHP version check could lead to the test being executed in environments where it's not supported, potentially causing false negatives. It's important to ensure that the functionalities tested are compatible across the PHP versions supported by this extension.tests/incremental_compress_add_basic.phpt (2)
- 9-12: The introduction of
$modeTypeswithBROTLI_PROCESSandBROTLI_FLUSHprovides a good approach to test different compression modes. This change enhances the test's coverage by ensuring that both modes work as expected.- 14-27: Iterating through different compression modes and verifying the output against the uncompressed data is a robust testing strategy. However, ensure that the
brotli_compress_initandbrotli_compress_addfunctions handle these modes correctly in all scenarios, including edge cases not covered by this test.tests/incremental_compress_add_alias.phpt (2)
- 9-12: Using
$modeTypesto test different compression modes in the namespace context is a good practice. It ensures that the alias functions work correctly with various modes.- 14-25: Testing the namespace alias functions with different compression modes is crucial for ensuring compatibility and functionality. It's important to verify that these tests cover all relevant scenarios and edge cases.
tests/incremental_uncompress_add_basic.phpt (2)
- 9-12: The use of
$modeTypesto test different decompression modes is a positive change, ensuring that the decompression process is thoroughly tested across different modes.- 14-33: Iterating through compression modes and testing the decompression process is a comprehensive approach. However, consider adding more diverse data patterns to ensure the decompression process is robust against various data types and structures.
tests/incremental_uncompress_add_alias.phpt (2)
- 9-12: The introduction of
$modeTypesfor testing different decompression modes in the namespace context is commendable. It ensures thorough testing of the alias functions across various modes.- 14-32: Testing namespace alias functions for decompression with different modes is essential. Ensure comprehensive coverage by including tests for edge cases and different data patterns.
tests/compress_add.phpt (1)
- 11-37: The test for
brotli_compress_addfunctionality appears comprehensive, covering multiple strings and checking for compression failures. However, it's important to also test with different compression modes (BROTLI_FLUSHandBROTLI_PROCESS) to ensure the function behaves as expected across all scenarios.brotli.stub.php (4)
- 17-17: Changing the default mode parameter in
brotli_compress_addtoBROTLI_FLUSHaligns with the PR objectives to modify the behavior of compression functions. Ensure this change is well-documented and communicated to users, as it may affect existing implementations.- 27-27: Modifying the default mode in
brotli_uncompress_addtoBROTLI_FLUSHis consistent with the changes made to the compression function. Similar to the compression function, document this change thoroughly to inform users about the new default behavior.- 44-44: The update to the namespace function
compress_addto useBROTLI_FLUSHas the default mode is a good practice for consistency across the API. Documentation and communication of this change are crucial for users utilizing the namespace functions.- 54-54: The change in the default mode for
uncompress_addin the namespace toBROTLI_FLUSHensures uniformity in the handling of compression modes. As with other changes, emphasize the importance of updating documentation and user guides.tests/incremental_compress_add_args.phpt (2)
- 12-32: Introducing
$modeTypesto iterate through different compression modes and adding checks for warnings related to out-of-range compression levels and modes is a significant improvement. This ensures that the function behaves correctly under various conditions and that users are warned about potential misconfigurations.- 56-88: The handling of warnings for out-of-range compression levels and modes is crucial for user awareness and error handling. Ensure that these warnings are clear and informative, guiding users to correct any misconfigurations effectively.
README.md (4)
- 162-168: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [165-181]
Updating the documentation to reflect the change in the default mode parameter for
brotli_compress_addtoBROTLI_FLUSHis essential for clarity and user understanding. Ensure that the documentation explicitly states the impact of this change on existing codebases and provides guidance on adjusting to the new default behavior.
- 205-211: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [208-224]
Similarly, updating the documentation for
brotli_uncompress_addto indicate the new default mode ofBROTLI_FLUSHis crucial. Documentation should guide users on how this change affects decompression behavior and any necessary adjustments in their code.
- 240-242: The documentation updates for namespace functions
compress_addanduncompress_addto reflect the new default mode ofBROTLI_FLUSHare well done. It's important to ensure that these updates are comprehensive and provide clear guidance for users on adapting to the changes.- 296-306: The examples provided for incremental compression and decompression using
BROTLI_FLUSHas the mode are helpful for users to understand the practical application of these changes. Ensure that these examples cover a wide range of use cases to demonstrate the versatility of the updated functionality.brotli.c (5)
- 906-907: The addition of the
BROTLI_FLUSHconstant aligns with the PR objectives, providing more control over the compression process.- 1091-1091: The change in the default value of
modeinbrotli_compress_addfromBROTLI_OPERATION_PROCESStoBROTLI_OPERATION_FLUSHis consistent with the PR objectives. This modification is crucial for allowing the function to flush the compression buffer, which can be beneficial in streaming scenarios.- 1271-1271: Similarly, the change in the default value of
modeinbrotli_uncompress_addfromBROTLI_OPERATION_PROCESStoBROTLI_OPERATION_FLUSHis consistent with the PR objectives. However, it's important to note that for decompression, the mode change might not have a direct impact sinceBROTLI_OPERATION_FLUSHis more relevant to the compression process. This change might be intended for consistency or future use cases.- 1130-1134: Proper error handling is implemented in
brotli_compress_addwhenBrotliEncoderCompressStreamfails. The allocated buffer and the smart string are correctly freed before returningFALSE, preventing memory leaks.- 1271-1271: In
brotli_uncompress_add, the function returns an empty string immediately ifin_sizeis less than or equal to 0 andmodeis notBROTLI_OPERATION_FINISH. This early return is efficient but should ensure that it doesn't skip necessary cleanup or finalization steps that might be added in the future.
| const uint8_t *next_in = in_buf; | ||
| size_t available_in = in_size; | ||
|
|
||
| if (in_size > 0) { | ||
| while (available_in) { | ||
| size_t available_out = buffer_size; | ||
| uint8_t *next_out = buffer; | ||
| if (BrotliEncoderCompressStream(ctx->encoder, | ||
| mode, | ||
| &available_in, | ||
| &next_in, | ||
| &available_out, | ||
| &next_out, | ||
| 0)) { | ||
| buffer_used = (size_t)(next_out - buffer); | ||
| if (buffer_used) { | ||
| smart_string_appendl(&out, buffer, buffer_used); | ||
| } | ||
| } else { | ||
| efree(buffer); | ||
| smart_string_free(&out); | ||
| php_error_docref(NULL TSRMLS_CC, E_WARNING, | ||
| "Brotli incremental compress failed\n"); | ||
| RETURN_FALSE; | ||
| while (available_in || BrotliEncoderHasMoreOutput(ctx->encoder)) { | ||
| size_t available_out = buffer_size; | ||
| uint8_t *next_out = buffer; | ||
| if (BrotliEncoderCompressStream(ctx->encoder, | ||
| mode, | ||
| &available_in, | ||
| &next_in, | ||
| &available_out, | ||
| &next_out, | ||
| 0)) { | ||
| buffer_used = (size_t)(next_out - buffer); | ||
| if (buffer_used) { | ||
| smart_string_appendl(&out, buffer, buffer_used); | ||
| } | ||
| } else { | ||
| efree(buffer); | ||
| smart_string_free(&out); | ||
| php_error_docref(NULL TSRMLS_CC, E_WARNING, | ||
| "Brotli incremental compress failed\n"); | ||
| RETURN_FALSE; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1343]
The provided code does not include modifications or additions to the test suite. It's crucial to have tests that cover the new functionality and changes introduced in this PR, especially for the new constants and the modified behavior of brotli_compress_add and brotli_uncompress_add functions.
Would you like assistance in creating tests for the new features and changes introduced in this PR?
Summary by CodeRabbit
New Features
Bug Fixes
Tests