Conversation
…ariable and adding compression coding check
WalkthroughAdds DCZ (dictionary-aware zstd) encoding to the PHP zstd extension: detects "dcz" Accept-Encoding, computes/verifies dictionary SHA-256 via Available-Dictionary header, prefixes DCZ outputs with a digest header, updates response headers, and adds four PHPT tests plus .gitattributes entries for .zstd and .dic files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PHP_CGI
participant ZSTD_Handler
Client->>PHP_CGI: HTTP Request (Accept-Encoding includes zstd/dcz, Available-Dictionary?)
PHP_CGI->>ZSTD_Handler: start compression with configured dictionary
ZSTD_Handler->>ZSTD_Handler: detect encodings (zstd/dcz)
alt DCZ requested and dictionary present
ZSTD_Handler->>ZSTD_Handler: load dict, compute SHA-256, base64-encode
ZSTD_Handler->>PHP_CGI: compare with Available-Dictionary header
alt digest matches
ZSTD_Handler->>PHP_CGI: compress, prefix 40-byte DCZ header
PHP_CGI->>Client: Response (Content-Encoding: dcz, Vary: Accept-Encoding, Available-Dictionary)
else mismatch or missing
ZSTD_Handler->>PHP_CGI: emit warning, disable DCZ
PHP_CGI->>Client: Response (Content-Encoding: zstd, Vary: Accept-Encoding)
end
else only zstd
ZSTD_Handler->>PHP_CGI: compress normally (zstd)
PHP_CGI->>Client: Response (Content-Encoding: zstd, Vary: Accept-Encoding)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-04-20T19:39:56.258ZApplied to files:
🔇 Additional comments (9)
✨ 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 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: 8
🧹 Nitpick comments (3)
tests/ob_dcz_001.phpt (1)
14-15: Minor: Prefer DIR for readabilityEquivalent on PHP 7+, consistent with modern PHPT style.
-ini_set('zstd.output_compression_dict', dirname(__FILE__) . '/data.dic'); +ini_set('zstd.output_compression_dict', __DIR__ . '/data.dic');tests/ob_dcz_002.phpt (1)
1-2: Clarify test titleBoth 002 and 003 use the same title. Consider making this say “not found available-dictionary” to distinguish from the mismatch case.
tests/ob_dcz_003.phpt (1)
1-2: Clarify test titleRename to reflect the mismatch case (requested vs actual digest).
---TEST-- -output handler: dcz: invalid available-dictionary +--TEST-- +output handler: dcz: invalid available-dictionary (mismatch)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitattributes(1 hunks)tests/ob_dcz_001.phpt(1 hunks)tests/ob_dcz_002.phpt(1 hunks)tests/ob_dcz_003.phpt(1 hunks)tests/ob_dcz_004.phpt(1 hunks)zstd.c(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-04-20T19:39:56.258Z
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.
Applied to files:
.gitattributestests/ob_dcz_003.phpttests/ob_dcz_004.phpttests/ob_dcz_002.phptzstd.c
⏰ 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). (18)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: ci (8.1, x86, nts, vs16, false, apcu)
- GitHub Check: ci (8.1, x86, ts, vs16, true)
- GitHub Check: ci (8.1, x86, nts, vs16, true)
- GitHub Check: ci (8.2, x86, ts, vs16, true)
- GitHub Check: ci (8.2, x86, nts, vs16, true, apcu)
- GitHub Check: ci (8.1, x64, ts, vs16, false, apcu)
- GitHub Check: ci (8.1, x64, nts, vs16, true)
- GitHub Check: ci (8.2, x64, ts, vs16, true)
- GitHub Check: ci (8.2, x86, nts, vs16, false, apcu)
- GitHub Check: ci (8.2, x64, ts, vs16, false)
- GitHub Check: ci (8.2, x64, nts, vs16, true)
- GitHub Check: ci (8.3, x86, nts, vs16, false, apcu)
- GitHub Check: ci (8.3, x64, nts, vs16, true, apcu)
- GitHub Check: ci (8.3-zts, php)
- GitHub Check: ci (8.3, apcu, php)
- GitHub Check: ci (8.3, php)
- GitHub Check: ci (8.4, apcu, php)
🔇 Additional comments (9)
tests/ob_dcz_002.phpt (1)
19-20: LGTM: Expected warning messageMessage pattern matches the “no Available-Dictionary header” scenario while requesting dcz.
tests/ob_dcz_003.phpt (1)
21-22: LGTM: Warning message covers digest mismatchFormat string with %s for actual digest is appropriate.
tests/ob_dcz_004.phpt (1)
20-21: External fixture confirmed present
The filetests/files/ob_data.zstdexists in the repository; no further action is required.zstd.c (6)
31-33: LGTM!The additional includes are appropriate for the SHA-256 hashing and base64 encoding functionality required for DCZ dictionary verification.
86-86: LGTM!The 32-byte digest field correctly matches the SHA-256 output size, and proper initialization with memset ensures no uninitialized memory.
Also applies to: 114-114
1261-1262: LGTM!Well-designed bit flags that allow for tracking multiple encoding types simultaneously.
1281-1284: LGTM!The encoding detection correctly identifies both "zstd" and "dcz" encodings from the Accept-Encoding header and properly sets the corresponding bit flags.
1376-1380: LGTM!The output handler correctly:
- Validates that at least one encoding is enabled before proceeding
- Properly constructs the 36-byte DCZ header with marker byte, "DCZ" identifier, and dictionary digest
- Allocates the correct buffer size accounting for the header overhead
Also applies to: 1403-1414
1496-1502: LGTM!The HTTP response headers are correctly configured:
- Proper Content-Encoding header based on the active encoding
- Appropriate Vary headers to ensure correct cache behavior, including Available-Dictionary for DCZ
Also applies to: 1521-1532
kjdev
force-pushed
the
experimental/dcz
branch
2 times, most recently
from
fa55acc to
780319a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores