[#492] Prevent Request bootstrap circular dependency via UploadedFile lazy init

[armanist]

Closes #492

Summary by CodeRabbit

• Refactor
  • Improved efficiency of file upload operations through internal optimization.

[coderabbitai]

Warning

Rate limit exceeded

@armanist has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 51 minutes and 58 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d75851a1-8761-4abe-ad7c-e2d92edf388d

📥 Commits

Reviewing files that changed from the base of the PR and between 3362f6b and e512b45.

📒 Files selected for processing (1)

• src/Storage/UploadedFile.php

📝 Walkthrough

Walkthrough

UploadedFile now defers filesystem adapter resolution and allowed MIME type configuration loading from construction to the save() method, eliminating eager initialization side effects that caused circular dependency issues during request bootstrap.

Changes

Cohort / File(s)	Summary
Lazy Initialization Pattern src/Storage/UploadedFile.php	Constructor side effects removed by deferring filesystem adapter resolution and MIME type config loading until save() is called. New getLocalFileSystem() helper method provides lazy access to the local filesystem. Caching via $mimeTypesLoaded boolean ensures config loads only once. Call sites updated to use helper method instead of direct property access.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A hop and a bound, through DI we go,
No circles now form in the bootstrap flow,
Lazy loaders gather, side effects fade,
The filesystem waits—when needed, we've made! 🌿

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title directly references issue #492 and clearly summarizes the main change: introducing lazy initialization in UploadedFile to prevent a circular dependency during Request bootstrap.
Linked Issues check	✅ Passed	The code changes implement all acceptance criteria from issue #492: lazy initialization of filesystem and config (deferred until save()), no constructor side effects, and preservation of existing upload behavior.
Out of Scope Changes check	✅ Passed	All changes are narrowly scoped to UploadedFile and directly address the circular dependency issue. No unrelated refactoring or broader architecture changes are present.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 58 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.73%. Comparing base (35b67fa) to head (e512b45).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Storage/UploadedFile.php	79.31%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #493   +/-   ##
=========================================
  Coverage     90.73%   90.73%           
- Complexity     2912     2917    +5     
=========================================
  Files           252      252           
  Lines          7670     7683   +13     
=========================================
+ Hits           6959     6971   +12     
- Misses          711      712    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

🧹 Nitpick comments (3)

src/Storage/UploadedFile.php (3)

257-295: 💤 Low value

Missing LoaderException in @throws annotation.

The ensureAllowedMimeTypesLoaded() method (called at line 266) can throw LoaderException, but this is not documented in the save() method's @throws annotation.

📝 Proposed fix

     * `@param` bool $overwrite
     * `@return` bool
-    * `@throws` FileUploadException|FileSystemException|ImageResizeException|BaseException|ReflectionException
+    * `@throws` FileUploadException|FileSystemException|ImageResizeException|BaseException|ReflectionException|LoaderException
     */
    public function save(string $dest, bool $overwrite = false): bool

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Storage/UploadedFile.php` around lines 257 - 295, The save() method's
PHPDoc is missing LoaderException which can be thrown by
ensureAllowedMimeTypesLoaded(); update the save() docblock to include
LoaderException in the `@throws` list so callers and static tools know this method
can propagate that exception (reference: method save(), call to
ensureAllowedMimeTypesLoaded()).

---

377-398: 💤 Low value

Missing FileSystemException in @throws annotation.

The method throws FileSystemException::notInstanceOf() at line 389, but this exception type is not documented in the @throws annotation.

📝 Proposed fix

    /**
-    * `@throws` ConfigException|DiException|BaseException|ReflectionException
+    * `@throws` ConfigException|DiException|BaseException|ReflectionException|FileSystemException
     */
    private function getLocalFileSystem(): LocalFilesystemAdapterInterface

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Storage/UploadedFile.php` around lines 377 - 398, The docblock for
getLocalFileSystem() is missing the FileSystemException that can be thrown by
FileSystemException::notInstanceOf() — update the method PHPDoc `@throws` to
include FileSystemException (either fully-qualified or ensure it's imported) so
the annotation matches the actual thrown exception from getLocalFileSystem().

---

145-152: 💤 Low value

Consider documenting potential exceptions in getName() and getExtension().

With the lazy initialization change, getName() and getExtension() can now throw exceptions from getLocalFileSystem() (e.g., ConfigException, DiException, FileSystemException). While this is unlikely to affect normal usage, callers that only retrieve file metadata without calling save() may encounter unexpected exceptions if the filesystem cannot be resolved.

This is a minor documentation concern since the primary use case (constructing then saving) is unaffected, and the PR correctly addresses the circular dependency issue.

Also applies to: 183-190

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Storage/UploadedFile.php` around lines 145 - 152, Document that getName()
and getExtension() can propagate exceptions from getLocalFileSystem(): add
PHPDoc `@throws` annotations to both methods (getName and getExtension) listing
the possible exceptions (e.g., ConfigException, DiException,
FileSystemException) so callers know these methods may throw when filesystem
resolution fails; update the PHPDoc directly above the getName() and
getExtension() method declarations and keep the descriptions concise and
consistent.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/Storage/UploadedFile.php`:
- Around line 257-295: The save() method's PHPDoc is missing LoaderException
which can be thrown by ensureAllowedMimeTypesLoaded(); update the save()
docblock to include LoaderException in the `@throws` list so callers and static
tools know this method can propagate that exception (reference: method save(),
call to ensureAllowedMimeTypesLoaded()).
- Around line 377-398: The docblock for getLocalFileSystem() is missing the
FileSystemException that can be thrown by FileSystemException::notInstanceOf() —
update the method PHPDoc `@throws` to include FileSystemException (either
fully-qualified or ensure it's imported) so the annotation matches the actual
thrown exception from getLocalFileSystem().
- Around line 145-152: Document that getName() and getExtension() can propagate
exceptions from getLocalFileSystem(): add PHPDoc `@throws` annotations to both
methods (getName and getExtension) listing the possible exceptions (e.g.,
ConfigException, DiException, FileSystemException) so callers know these methods
may throw when filesystem resolution fails; update the PHPDoc directly above the
getName() and getExtension() method declarations and keep the descriptions
concise and consistent.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3477e973-1407-447b-a57f-c5a1330c11f2

📥 Commits

Reviewing files that changed from the base of the PR and between 35b67fa and 3362f6b.

📒 Files selected for processing (1)

• src/Storage/UploadedFile.php

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3362f6b455

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".