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] Add support for Storage::url() for the Ftp driver #31258

Merged
merged 1 commit into from Jan 28, 2020

Conversation

@shadoWalker89
Copy link
Contributor

shadoWalker89 commented Jan 28, 2020

Currently it's not possible to generate an url for files that are stored on a ftp disk using the Storage::url()
This PR allows to generate an url by adding a url key in the configuration of the disk the same way local and s3 do.

Motiviation

I have an application that is running in production for years since laravel 5.2 and i'm using the local driver to store files on the server. This app has taken a lot of space and i need to move the files to a remote ftp server.
Adding support for url generation with Storage::url() for the ftp driver will allow me to keep my code as is and not change a single line of code.
I just need to move the files to the new ftp server and everything will work the same, i don't even need to update the database since i'm storing only relative paths in the db.

Other then my use case this will allow to be consistent and allow for url generation using the Storage::url() no matter what driver the developper is using.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 28, 2020

Hmm. I'm 👎 here. I don't think we need this in the framework core. I have a package that lets you get at all of the flysystem drivers if you need it (it actually predates flysystem being in the laravel core).

@shadoWalker89

This comment has been minimized.

Copy link
Contributor Author

shadoWalker89 commented Jan 28, 2020

This technique is already used with the local and s3 drivers that are not in the core.
As a matter of fact laravel does not have any adapter in the core, even for the local one. It uses the adapters provided by league/flysystem
Problem with league adapters is that they do not provide the ability to get the url, this is being done on the laravel side.
I know it's possible to extend the ftp adapter and add a getUrl method there but i think this is a better way to go about it because this will allow laravel to support url generation for ftp driver out of the box without relying on installing some third party package.
Generating an url is an essential part of the equation, i upload file to ftp to then be able to use it, view it in the browser, download it from the browser.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 28, 2020

As a matter of fact laravel does not have any adapter in the core, even for the local one. It uses the adapters provided by league/flysystem

I know. That's what I mean by "in the core".

@shadoWalker89

This comment has been minimized.

Copy link
Contributor Author

shadoWalker89 commented Jan 28, 2020

As a matter of fact laravel does not have any adapter in the core, even for the local one. It uses the adapters provided by league/flysystem

I know. That's what I mean by "in the core".

I think you missed this part 😄

This technique is already used with the local and s3 drivers that are not in the core.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 28, 2020

I think you missed this part 😄

Definitely not. I know how the laravel flysystem stuff works!

@shadoWalker89

This comment has been minimized.

Copy link
Contributor Author

shadoWalker89 commented Jan 28, 2020

@GrahamCampbell Do you really think it's a good think to allow people to upload a file via ftp and then tell them that they need to download some package just to be able to generate urls ?
Generating urls should be supported out of the box, it's an essential feature of dealing with files.
Maybe we disagree on the implementation details, but at the end a framework should allow the developper to upload and generate urls without any extra dependencies.

@shadoWalker89

This comment has been minimized.

Copy link
Contributor Author

shadoWalker89 commented Jan 28, 2020

I'm open for suggestions on how to proceed on adding support for url generation for ftp.
There are two ways :
1- The way this PR does it which the same way is done for the other drivers local and s3
2- Laravel create it's own adapters that extend the league ones, which is totally unnecessary because that is why Illuminate\Filesystem\FilesystemAdapter exists in the first place to wrap the league ones and provide functionality like this.

But this does not change the fact that generating urls is essential across all the drivers out of the box using the framework Storage::url()

BTW laravel flysystem is already coupled with the league one before the PR

Illuminate\Filesystem\FilesystemAdapter constructor
image

Creating ftp driver is based on the league/flysystem Ftp class, so laravel already

(predates flysystem being in the laravel core.)

image

@taylorotwell taylorotwell merged commit 06abd3a into laravel:6.x Jan 28, 2020
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
@shadoWalker89 shadoWalker89 deleted the shadoWalker89:ftp_storage_url branch Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.