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

[9.x] Optionally cascade thrown Flysystem exceptions #41308

Merged
merged 7 commits into from
Mar 3, 2022

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Mar 3, 2022

This PR gives the ability to optionally cascade thrown Flysystem exceptions with a throws_exceptions config option.

There's a concern with this: operations like the delete method will hard-fail now when passing multiple paths. Any paths that haven't been reached yet won't be deleted yet. Not sure if this is something we should work around?

This PR is opt-in and non-breaking.

Related: #41269

$this->fail('Exception was not thrown.');
}

/** @requires OS Linux|Darwin */
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't get this test to work on Windows but I'd assume it's related to how I'm creating the directory so I expect exceptions throwing works fine on it 😅

Copy link
Contributor

@Jubeki Jubeki Mar 3, 2022

Choose a reason for hiding this comment

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

May have something todo with the different directory separator:
Linux/MacOS: /
Windows: \

Maybe use: DIRECTORY_SEPARATOR

EDIT:
Reference: https://www.php.net/manual/en/dir.constants.php

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that but it didn't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah seems like windows ignores the permissions which are set by mkdir: https://www.php.net/manual/en/function.mkdir.php

Copy link
Contributor

Choose a reason for hiding this comment

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

You could create a 400 permission read-only file and attempt to overwrite it. I verified this passes on Windows 10:

public function testThrowExceptionsForPut()
{
    $this->filesystem->write('foo.txt', 'Hello World');

    chmod(__DIR__.'/tmp/foo.txt', 0400);

    $adapter = new FilesystemAdapter($this->filesystem, $this->adapter, ['throws_exceptions' => true]);

    try {
        $adapter->put('foo.txt', 'Hello World!');

        $this->fail('UnableToWriteFile exception was not thrown.');
    } catch (UnableToWriteFile $e) {
        $this->assertTrue(true);
    } finally {
        chmod(__DIR__.'/tmp/foo.txt', 0600);
    }
}

It needs chmod(__DIR__.'/tmp/foo.txt', 0600) before the tearDown() deletion of ./tmp which will fail if the subdirectory contains a read-only file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@derekmd that didn't seem to work so I'm reverting that. Keeping tests on Linux/macOS for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it failed because the wip commit used old config key throws_exceptions after Taylor renamed it to throw.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah could be yeah. Gonna leave it now myself but would appreciate a PR if anyone wants to.

@ankurk91
Copy link
Contributor

ankurk91 commented Mar 3, 2022

@driesvints

Thanks for the PR. 😍

Please guide us where to specify the config option throws_exceptions in config/filesystem.php, an example would be helpful.

@driesvints
Copy link
Member Author

@ankurk91 no problem. You'd add this to any disk's config array:

'disks' => [
    'local' => [
        'driver' => 'local',
        'root' => storage_path('app'),
        'throws_exceptions' => true,
    ],
],

@taylorotwell
Copy link
Member

Renamed this config option to just throw. 👍

This reverts commit dc22a28.
@taylorotwell taylorotwell merged commit f119070 into 9.x Mar 3, 2022
@taylorotwell taylorotwell deleted the filesystem-throw-exceptions branch March 3, 2022 14:52
@browner12
Copy link
Contributor

is there a way to conditionally @throw an exception in the docblock? otherwise the IDE gets confused why we're trying to catch an exception it doesn't think is being thrown.

@driesvints
Copy link
Member Author

@browner12 don't think so no

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.

7 participants