built-in mime type exclusion list - #104
Conversation
📝 WalkthroughWalkthroughThe extension adds built-in MIME types that bypass Zstandard compression. It checks built-in and configured exclusion lists, reports the built-in list through ChangesMIME exclusion support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Startup as Output compression startup
participant Matcher as php_zstd_output_mimetype_excluded
participant Lists as Built-in and configured exclusion lists
Startup->>Matcher: Check response MIME type
Matcher->>Lists: Test both exclusion lists
Lists-->>Matcher: Return match result
Matcher-->>Startup: Skip compression when matched
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zstd.c (1)
1561-1565: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the built-in exclusion list.
The supplied
tests/ob_exclude_002.phptsetszstd.output_compression_exclude_types=image/*, so it exercises the configured list. It would still pass if the built-in exclusion call were removed. Add a case with an empty INI exclusion setting andContent-Type: image/png.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd.c` around lines 1561 - 1565, Add a regression case covering the built-in exclusion list in tests/ob_exclude_002.phpt or its related test suite: configure zstd.output_compression_exclude_types to an empty value, send a response with Content-Type: image/png, and assert the expected exclusion behavior so the test fails if php_zstd_output_mimetype_excluded(ZSTD_MIMETYPE_EXCLUDE) is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@php_zstd_mimetype_exclude.h`:
- Line 23: Update the built-in RAR MIME exclusions in the MIME type list to use
the registered application/vnd.rar value, removing application/x-vnd.rar;
include application/x-rar-compressed only if legacy server responses must remain
supported.
---
Nitpick comments:
In `@zstd.c`:
- Around line 1561-1565: Add a regression case covering the built-in exclusion
list in tests/ob_exclude_002.phpt or its related test suite: configure
zstd.output_compression_exclude_types to an empty value, send a response with
Content-Type: image/png, and assert the expected exclusion behavior so the test
fails if php_zstd_output_mimetype_excluded(ZSTD_MIMETYPE_EXCLUDE) is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 775656ab-802e-4d31-a18f-7457fe4d72dd
📒 Files selected for processing (4)
README.mdphp_zstd_mimetype_exclude.htests/ob_exclude_001.phptzstd.c
💤 Files with no reviewable changes (1)
- tests/ob_exclude_001.phpt
38d21a7 to
bc9a945
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ob_exclude_002.phpt`:
- Line 16: Update the test setup in tests/ob_exclude_002.phpt so
zstd.output_compression_exclude_types no longer matches the image/svg
Content-Type, allowing the test to exercise the built-in exclusion list. Use a
non-matching configured value while preserving configured wildcard coverage in a
separate test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 466cfe81-0fbc-41f8-bc05-e960e0546544
📒 Files selected for processing (5)
README.mdphp_zstd_mimetype_exclude.htests/ob_exclude_001.phpttests/ob_exclude_002.phptzstd.c
💤 Files with no reviewable changes (1)
- tests/ob_exclude_001.phpt
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- zstd.c
- php_zstd_mimetype_exclude.h
Hey
I thought it would be a good idea to ship with a built-in list of common and/or obvious cases of these mime type exclusions. With this implemented, user only has to add any types, which might be missing from the built-in one.
List of these built-in exclusions is also printed to phpinfo();
One of the associated tests was modified to use the built-in list, while another one was modified to use the ini list.
I placed the list to a separate header, so it can be easily be replaced too, if someone wants to compile this with their own list.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation