Refactor/zend parse parameters #84
Conversation
WalkthroughThe changes update parameter parsing in several zstd compression-related functions to use modern Zend macros, removing legacy PHP version checks and manual error handling. Corresponding test files are updated to expect more explicit and detailed warning messages reflecting these improvements in error reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PHP_Extension
participant Zend_API
User->>PHP_Extension: Call zstd_compress()/zstd_uncompress() with arguments
PHP_Extension->>Zend_API: Parse parameters using ZEND_PARSE_PARAMETERS_START
alt Parameters valid
Zend_API-->>PHP_Extension: Parameters extracted
PHP_Extension-->>User: Proceed with compression/decompression
else Parameters invalid
Zend_API-->>PHP_Extension: Emit detailed warning (parameter index, type)
PHP_Extension-->>User: Return false
end
Poem
✨ Finishing Touches
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. 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: 0
♻️ Duplicate comments (4)
zstd.c (4)
375-378: Same concern as earlier about the availability ofZEND_PARSE_PARAMETERS_END_EXon the lowest supported PHP-7 version.
466-470: See first comment – applies here as well.
617-621: Same macro-compatibility consideration.
657-666: Same remark regarding macro presence; otherwise the refactor looks clean.
🧹 Nitpick comments (1)
zstd.c (1)
718-737: Remove unused local variable to avoid compiler warnings
zend_object *context;is declared but never used. Compilers (esp. with-Werror) will complain.- zend_object *context;No other code refers to
context, so it can be deleted safely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/002.phpt(1 hunks)tests/005.phpt(1 hunks)zstd.c(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
zstd.c (1)
Learnt from: thekid
PR: kjdev/php-ext-zstd#77
File: zstd.stub.php:13-22
Timestamp: 2025-04-20T19:39:56.258Z
Learning: The php-ext-zstd library aims to maintain compatibility with PHP 7, so PHP 8-specific syntax features like union types should be avoided in favor of PHPDoc annotations.
🔇 Additional comments (3)
zstd.c (1)
340-344: ConfirmZEND_PARSE_PARAMETERS_END_EXexists on all supported PHP-7 targetsGood call switching to the
_EX(RETURN_FALSE)variant – it keeps the fast-return behaviour explicit.
Please double-check that this macro is available all the way down to the minimum PHP 7.x version you claim to support (7.0/7.1). If not, a small#if !defined(ZEND_PARSE_PARAMETERS_END_EX)shim will be required.tests/002.phpt (1)
29-29: Expectation update aligns with new Zend-parsing error – looks goodThe revised message matches the behaviour after switching to modern parameter parsing.
tests/005.phpt (1)
29-33: Updated expectations are consistent with new parsing semanticsThe integer argument now passes the parser and fails inside
zstd_uncompress, so the expected warning is correct.
Summary by CodeRabbit