fix: skip translation files exceeding a size limit#143
Conversation
📝 WalkthroughWalkthroughCollector now skips translation files larger than 30 MB during recursive and non-recursive discovery, logs a warning for rejected files, and adds coverage using small and sparse oversized XLF fixtures. ChangesTranslation file size filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Collector
participant Filesystem
participant Logger
participant Detector
Collector->>Filesystem: Check candidate file size
Filesystem-->>Collector: Return file size
Collector->>Logger: Warn when size exceeds 30 MB
Collector->>Detector: Map files within the size limit
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 PHPStan (2.2.2)PHPStan was skipped because the config uses disallowed 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 review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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)
src/FileDetector/Collector.php (1)
134-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the recursive scan path.
The size-limit guard is applied in both the non-recursive (line 138) and recursive (line 163) code paths, but the new test only exercises the non-recursive path (default
$recursive = false). Adding a recursive variant ensures the compound condition in the recursive iterator is also validated against oversized files.Also applies to: 161-163
🤖 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 `@src/FileDetector/Collector.php` around lines 134 - 138, Add a test variant that invokes the recursive scan path by setting recursive to true, using an oversized file with a supported extension and asserting it is excluded. Keep the existing non-recursive coverage and target the relevant Collector test and recursive scanning 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 `@src/FileDetector/Collector.php`:
- Around line 184-193: Update the size check in the file collection method
containing the filesize() call to handle a false result explicitly: log a
warning identifying the file and inability to determine its size, then return
false. Preserve the existing warning and rejection behavior for files exceeding
MAX_FILE_SIZE.
---
Nitpick comments:
In `@src/FileDetector/Collector.php`:
- Around line 134-138: Add a test variant that invokes the recursive scan path
by setting recursive to true, using an oversized file with a supported extension
and asserting it is excluded. Keep the existing non-recursive coverage and
target the relevant Collector test and recursive scanning 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: 40eac3d4-778a-4b98-ba05-74c7c850ae0d
📒 Files selected for processing (2)
src/FileDetector/Collector.phptests/src/FileDetector/CollectorTest.php
| $size = filesize($file); | ||
| if (false !== $size && $size > self::MAX_FILE_SIZE) { | ||
| $this->logger?->warning( | ||
| sprintf('Skipping file exceeding the maximum size of %d bytes: %s', self::MAX_FILE_SIZE, $file), | ||
| ); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle filesize() failure explicitly.
When filesize() returns false (e.g., permission denied, file removed between is_file() and filesize()), the file silently passes the size guard with no log entry. This defeats the memory-safety purpose of the limit. Consider logging a warning and returning false (or at minimum logging for observability) when the size cannot be determined.
🛡️ Proposed fix
private function isWithinSizeLimit(string $file): bool
{
$size = filesize($file);
- if (false !== $size && $size > self::MAX_FILE_SIZE) {
+ if (false === $size) {
+ $this->logger?->warning(
+ sprintf('Unable to determine file size, skipping: %s', $file),
+ );
+
+ return false;
+ }
+
+ if ($size > self::MAX_FILE_SIZE) {
$this->logger?->warning(
sprintf('Skipping file exceeding the maximum size of %d bytes: %s', self::MAX_FILE_SIZE, $file),
);📝 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.
| $size = filesize($file); | |
| if (false !== $size && $size > self::MAX_FILE_SIZE) { | |
| $this->logger?->warning( | |
| sprintf('Skipping file exceeding the maximum size of %d bytes: %s', self::MAX_FILE_SIZE, $file), | |
| ); | |
| return false; | |
| } | |
| return true; | |
| $size = filesize($file); | |
| if (false === $size) { | |
| $this->logger?->warning( | |
| sprintf('Unable to determine file size, skipping: %s', $file), | |
| ); | |
| return false; | |
| } | |
| if ($size > self::MAX_FILE_SIZE) { | |
| $this->logger?->warning( | |
| sprintf('Skipping file exceeding the maximum size of %d bytes: %s', self::MAX_FILE_SIZE, $file), | |
| ); | |
| return false; | |
| } | |
| return true; |
🤖 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 `@src/FileDetector/Collector.php` around lines 184 - 193, Update the size check
in the file collection method containing the filesize() call to handle a false
result explicitly: log a warning identifying the file and inability to determine
its size, then return false. Preserve the existing warning and rejection
behavior for files exceeding MAX_FILE_SIZE.
1 participant
Summary
file_get_contentsand no upper bound, so a very large (or sparsely inflated) translation file could exhaust memory when scanning untrusted directories.Changes
src/FileDetector/Collector.php— add aMAX_FILE_SIZEguard applied in both the recursive and non-recursive scan pathstests/src/FileDetector/CollectorTest.php— assert an oversized (sparse) file is skipped while a normal file is still collectedSummary by CodeRabbit
Bug Fixes
Tests