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

[6.x] Fix the Filesystem manager's exception on unsupported driver #30331

Merged
merged 1 commit into from Oct 21, 2019

Conversation

@arcanedev-maroc
Copy link
Contributor

arcanedev-maroc commented Oct 18, 2019

This will fix the exception message on unsupported driver.

For example Storage::disk('unsupported-disk');

Before
Driver [] is not supported.

After
Driver [unsupported-disk] is not supported.

@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Oct 18, 2019

Neat 👍

@arcanedev-maroc arcanedev-maroc force-pushed the ARCANEFORKS:patch-filesystem_manager branch from 27dc80f to 734b69b Oct 18, 2019
@arcanedev-maroc

This comment has been minimized.

Copy link
Contributor Author

arcanedev-maroc commented Oct 18, 2019

@driesvints Done 👍

@arcanedev-maroc

This comment has been minimized.

Copy link
Contributor Author

arcanedev-maroc commented Oct 18, 2019

Note: Failed test not related

Hi @driesvints, i was wondering how to mock the config repository so i check the other tests.

I found that the $this->assert***() (PHPUnit asserts) are not called as a static method 👀

UPDATE
How to solve this https://travis-ci.org/laravel/framework/jobs/599655857#L589-L595 ? Return array instead of null ?

@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Oct 18, 2019

@arcanedev-maroc think the tests are definitely related and that the problem is just that you need to rewrite this line:

$app['config'] = ['filesystems.disks.unsupported-disk' => null];

to this?

$app['config'] = ['filesystems.disks' => []];
@arcanedev-maroc arcanedev-maroc force-pushed the ARCANEFORKS:patch-filesystem_manager branch from 734b69b to 286f701 Oct 18, 2019
@arcanedev-maroc

This comment has been minimized.

Copy link
Contributor Author

arcanedev-maroc commented Oct 18, 2019

@driesvints How about this change ?

@arcanedev-maroc

This comment has been minimized.

Copy link
Contributor Author

arcanedev-maroc commented Oct 18, 2019

Interesting, something changed in arrays for PHP 7.4 🤔

Update:

@driesvints Try this in PHP 7.3 and 7.4 👀

$config = null;

dd($config['name']);
@arcanedev-maroc arcanedev-maroc force-pushed the ARCANEFORKS:patch-filesystem_manager branch from a44d6ec to aa385e0 Oct 18, 2019
@arcanedev-maroc arcanedev-maroc force-pushed the ARCANEFORKS:patch-filesystem_manager branch from aa385e0 to 9a3e6a9 Oct 18, 2019
@arcanedev-maroc arcanedev-maroc requested a review from driesvints Oct 18, 2019
@netpok

This comment has been minimized.

Copy link
Contributor

netpok commented Oct 18, 2019

Related RFC about the null-array change

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Oct 21, 2019

Is this ready to merge? I don't know if the previous comments are saying there is a problem or what?

@taylorotwell taylorotwell merged commit 9a3e6a9 into laravel:6.x Oct 21, 2019
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arcanedev-maroc arcanedev-maroc deleted the ARCANEFORKS:patch-filesystem_manager branch Oct 21, 2019
@netpok

This comment has been minimized.

Copy link
Contributor

netpok commented Oct 21, 2019

@taylorotwell I just checked and I think this merge should be reverted, this changes the driver resolution.

When a disk named local have no driver it thrown an exception in the previous version (through multiple levels of non-existing array keys). With this merge it will resolve to local driver.

I think the correct solution would be to check against the existence of the driver field and throw an exception with Disk [name] has no configured driver instead of resolving by its name.

Also it will try to create drivers for non existing disks: if filesystems.php has no disk named local the Storage::disk('local') will still try to create a local disk (actually it will fail because it tries to pass a null to a method which has an array typehint, but that's an other story).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.