built-in mime type exclusion list - #104
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 |
![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 (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
bc9a945 to
c95cea4
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 `@README.md`:
- Around line 92-93: Update the README statement about locating the MIME-type
list through phpinfo() to specify that this visibility is available only in PHP
8 and later, and document an alternative inspection method for older supported
PHP versions if one exists.
🪄 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: 505f7422-cf4c-487a-a4ad-524568dd5011
📒 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)
- php_zstd_mimetype_exclude.h
- tests/ob_exclude_002.phpt
- zstd.c
c95cea4 to
9825967
Compare
9825967 to
1f704d2
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/info.phpt`:
- Line 79: Update the MIME-list extraction in tests/info.phpt to use the offset
matching the 42-byte $search prefix instead of skipping the first MIME
character; keep the existing explode-based parsing unchanged.
🪄 Autofix
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: 1b02083d-148f-4799-9429-296cf27ae517
📒 Files selected for processing (7)
README.mdpackage.xmlphp_zstd_mimetype_exclude.htests/info.phpttests/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 (5)
- package.xml
- php_zstd_mimetype_exclude.h
- README.md
- tests/ob_exclude_002.phpt
- zstd.c
| } | ||
| } | ||
| if ($mimeIndex) { | ||
| $types = explode(', ', substr($lines[$mimeIndex], 43)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Extract the MIME list at the correct offset.
Line 79 skips the first byte of the first MIME type. $search is 42 bytes long, but substr() starts at offset 43. The regex accepts the truncated value, so this test does not validate the first MIME type.
Proposed fix
- $types = explode(', ', substr($lines[$mimeIndex], 43));
+ $types = explode(', ', substr($lines[$mimeIndex], strlen($search)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $types = explode(', ', substr($lines[$mimeIndex], 43)); | |
| $types = explode(', ', substr($lines[$mimeIndex], strlen($search))); |
🤖 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 `@tests/info.phpt` at line 79, Update the MIME-list extraction in
tests/info.phpt to use the offset matching the 42-byte $search prefix instead of
skipping the first MIME character; keep the existing explode-based parsing
unchanged.
1f704d2 to
7006cb4
Compare
2 participants
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
type/*wildcards.Documentation