Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

New rule: Ensure access to defined filesystems #1989

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

aictur
Copy link

@aictur aictur commented Jul 8, 2024

  • Added or updated tests
  • Documented user facing changes

Changes

Added a rule that ensures access to a defined filesystem or 'disk' configured in config/filesystems.php when using Laravel Storage abstraction API.

Breaking changes

No breaking changes.

@szepeviktor
Copy link
Collaborator

This is very nice PR.

{
public function getStorage(): Storage
{
return Storage::disk('local');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to have two separate test files, you can add both lines in this test (rename the file of course, and there's no need for there to be a class) and just assert that the appropriate line throws the error:

The test file can be as simple as:

<?php

declare(strict_types=1);

use Illuminate\Support\Facades\Storage;

function test(): void
{
    Storage::disk('local');
    Storage::disk('this-is-not-defined');
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and maybe a variable: Storage::disk($_GET['disk']);

Comment on lines +31 to +33
public function __construct(protected ReflectionProvider $reflectionProvider)
{
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed and can be removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants