Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add a report/log option to filesystem exceptions without throwing #54212

Conversation

lotharthesavior
Copy link
Contributor

This PR introduces a new feature that allows users to configure whether filesystem operation errors should be logged without throwing exceptions.

Key Changes:

  • Added a report configuration option to the disk configuration in filesystems.php.
  • Enhanced the Filesystem Adapter to support logging errors.
  • Users can now:
    • To throw exceptions by setting "throw" => true in their disk configuration.
  • With this PR, Users will be able to:
    • To log errors without throwing exceptions by setting "report" => true.

Use Case:

This feature provides flexibility for users who want to monitor filesystem errors through logs instead of disrupting their application flow with exceptions. It’s particularly useful for non-critical operations where logging errors is sufficient or for monitoring/auditing purposes.

Example Configuration:

'disks' => [
    'local' => [
        'driver' => 'local',
        'root' => storage_path('app'),
        'throw' => false, // Suppress exceptions
        'report' => true,    // Enable error logging
    ],
],

Impact:

  • If throw is true, exceptions will still be thrown.
  • If throw is false and report is true, errors will be logged.
  • If neither is set, the current behavior remains unchanged.

This change maintains backward compatibility while providing more granular error-handling options for filesystem operations.

@taylorotwell
Copy link
Member

The report_if helper is defined in the framework and not in the support Composer package, so this would break for anyone using illuminate/filesystem without the framework.

@taylorotwell taylorotwell marked this pull request as draft January 16, 2025 19:30
@lotharthesavior
Copy link
Contributor Author

That’s great! I hadn’t noticed that dependency issue before. I fixed it by using the exception handler from the application container only if it's available. If it’s not, the code simply does nothing. This solution also preserves the dependency injection strategy for the adapter.

As for the code style, it’s not passing, but I read in the contribution docs that it’s not required at the code review stage since it’s automatically handled at the merge.

@lotharthesavior lotharthesavior marked this pull request as ready for review January 16, 2025 21:47
@taylorotwell taylorotwell merged commit 0b5ad67 into laravel:11.x Jan 17, 2025
37 of 38 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants