Add MIME Type Exclusions for Transparent Output Compression#91
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new INI option ChangesMIME Type Exclusion Configuration
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)brotli.cbrotli.c:5:10: fatal error: 'php.h' file not found ... [truncated 640 characters] ... tem" 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)
brotli.c (1)
384-432: ⚡ Quick winAdd PHPT coverage for exact/wildcard exclusions with parameterized
Content-Type.This new path should have explicit tests for: exact match (
application/pdf), wildcard (image/*), and exact match with parameters (text/html; charset=UTF-8) to prevent regressions in handler start logic.Also applies to: 554-557
🤖 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 `@brotli.c` around lines 384 - 432, Add PHPT tests exercising php_brotli_output_mimetype_excluded for (1) exact match (e.g. BROTLI_G(output_compression_exclude_types)="application/pdf" with SG(sapi_headers).mimetype="application/pdf"), (2) wildcard match (e.g. "image/*" matching "image/png"), and (3) exact match with parameters (e.g. SG(sapi_headers).mimetype="text/html; charset=UTF-8" matching "text/html"); ensure tests set BROTLI_G(output_compression_exclude_types) and SG(sapi_headers).mimetype and assert the function returns true for excluded types and false for non-matching types to prevent regressions in handler start logic.
🤖 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 `@brotli.c`:
- Around line 395-424: The code currently uses mimetype_len = strlen(mimetype)
so comparisons against entries in exclude (e.g. "text/html") fail when the
Content-Type has parameters like "text/html; charset=UTF-8"; change the logic to
compute mimetype_len as the length of the media type only (stop at the first ';'
or trailing whitespace), then use that trimmed mimetype_len for the existing
comparisons (including the wildcard branch that computes prefix_len and the
exact token_len match), updating any uses of mimetype_len in the block that
checks p/token_len and strncasecmp to compare only the media type portion.
---
Nitpick comments:
In `@brotli.c`:
- Around line 384-432: Add PHPT tests exercising
php_brotli_output_mimetype_excluded for (1) exact match (e.g.
BROTLI_G(output_compression_exclude_types)="application/pdf" with
SG(sapi_headers).mimetype="application/pdf"), (2) wildcard match (e.g. "image/*"
matching "image/png"), and (3) exact match with parameters (e.g.
SG(sapi_headers).mimetype="text/html; charset=UTF-8" matching "text/html");
ensure tests set BROTLI_G(output_compression_exclude_types) and
SG(sapi_headers).mimetype and assert the function returns true for excluded
types and false for non-matching types to prevent regressions in handler start
logic.
🪄 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
Run ID: 9de13b90-f53b-4373-be4a-049c35285bf6
📒 Files selected for processing (3)
README.mdbrotli.cphp_brotli.h
2 participants
Add MIME Type Exclusions for Transparent Output Compression
Summary
This patch adds a new INI directive:
The directive allows administrators to exclude selected MIME types from transparent Brotli output compression.
Examples
Supported Matching
Exact MIME matches
application/pdfapplication/zipWildcard family matches
image/*video/*audio/*Motivation
Many binary formats are already compressed and provide little or no benefit from additional Brotli compression while still consuming CPU resources.
This feature allows administrators to keep transparent output compression enabled while excluding selected content types.
Changes
brotli.output_compression_exclude_typesINI directiveSummary by CodeRabbit
New Features
brotli.output_compression_exclude_typesconfiguration to exclude specific MIME types from transparent Brotli output compression. Accepts comma-separated values with exact MIME matches andtype/*wildcard family patterns (case-insensitive).Documentation