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

[5.3] Use 'url_root' config value to generate disk url #16281

Merged
merged 2 commits into from
Nov 7, 2016

Conversation

shadoWalker89
Copy link
Contributor

image

The value of url_root will be used instead of /storage/

This is very useful in multi tenant applications, where each tenant should have it's own subdomain for serving files.

@GrahamCampbell GrahamCampbell changed the title Use 'url_root' config value to generate disk url [5.3] Use 'url_root' config value to generate disk url Nov 5, 2016
@shadoWalker89
Copy link
Contributor Author

Using the same technique, ftp disk could generate urls too.
Should i add it ?

@taylorotwell
Copy link
Member

If you already know the full URL, why even use the filesystem to generate it at all? Why not write your own helper to generate it?

@shadoWalker89
Copy link
Contributor Author

Since the purpose of Storage::url() is to generate the url to the file, why doesn't it then generate the correct url instead of returning the /storage/PathToTheFile ?

Also it's not always true that the files are being stored directly under /storage/ in the PROJECT_ROOT/public/

Also consider someone with this config

'disks' => [
    'public' => [
        'driver' => 'local',
        'root' => storage_path('app/public/some_directory/'),
        'visibility' => 'public'
    ]
]

In this case the Storage::url('file.txt') should return /storage/some_directory/file.txt and not /storage/file.txt

To fix this, we can then add the config url_root with the following value /storage/some_directory

Another use case is when the files are stored directly in public directory without using symbolic links

'disks' => [
    'public' => [
        'driver' => 'local',
        'root' => public_path('/uploads'),
        'visibility' => 'public',
    ]
]

Using Storage::url('file.txt') should return /uploads/file.txt and not /storage/file.txt

In addition we could use the same technique to generate urls with the FTP driver.

Laravel, is well know for flexibility and being able to customize parts of the framework.
Having the possibility to define url root seems logical to me in web applications, and it's better then having a default /storage/ that can not be easily changed.

After all adding one key of config is a lot better then making a new helper to generate urls.

@taylorotwell
Copy link
Member

But I'm saying you just shouldn't do either of those things. What's the point? It's just change for change's sake it seems like to me unless there is more information you have? Storing them in storage/public works fine. Storing them directly in public is a legitimate problem I don't even want to encourage because it breaks things in many types of deployment setups.

@shadoWalker89
Copy link
Contributor Author

It's not changing the directory name for fun.

Have two real world use cases:

First use case

In the first case i'm forced not to use symbolic links (uploads stored directly in the public), and use a folder other then /storage/ in the public folder.

I have a legacy application, that is going to be upgraded to laravel. The legacy application uploads are stored within /uploads/ in the root of the app folder.
After the laravel app is ready, the two apps will co-exist at the same time for one year and share the same uploads folder.
So what i'll end up doing is moving the legacy app to the public folder of the laravel app and have a legacy.the-app-domain.net point to it.

The intial domain will point the the public folder of the laravel app.
Using the url_root config, generating urls to files will be as easy as adding this line of config.
'url_root' => public_path('/legacy/uploads')

Second use case

Like i demonstrated in my initial message i have a multi tenant application. Each tenant has his own disk. The disk path is storage_path('app/public/tenantSubDirectory')

In this case the disk doesn't point directly to storage in the public folder but to storage/tenantSubDirectory

The reason for using a disk per tenant, is that it will make my code better.
I have a middleware that detect the tenant from the subdomain and set the default disk.
Which means in my code i can simply do Sotrage::put(), Storage::get(), Storage::url() and the file operations will be excuted on storage/tenantSubDirectory.

But if i use a single disk with this path storage_path('app/public') i'll have to add the tenant sub-directory in each operation, Sotrage::put($tenantSubDirectory . '/file.txt'), Storage::get($tenantSubDirectory . '/file.txt'), Storage::url($tenantSubDirectory . '/file.txt')


This is two uses cases that i'm dealing with right now, and maybe others could have similar or different ones in the future.

The PR allow for more flexibility without changing the default behavior. (so no harm is done)

On top of all that, the framework allows to set the default folder for storing sessions, cache, views, compiled which there is no need to change them. (I never changed them)
So why shouldn't it allow to change the url_root of the uploaded files ? Which seems something that could change in web apps more often then session, cache ...

@taylorotwell taylorotwell merged commit 0e1692e into laravel:5.3 Nov 7, 2016
@shadoWalker89 shadoWalker89 deleted the local_disk_url_root branch November 8, 2016 07:06
@tillkruss
Copy link
Collaborator

Should this be mentioned in the docs?

sebdesign added a commit to sebdesign/laravel-mediable that referenced this pull request Nov 24, 2016
Add the ability to set a custom url for a disk.
This feature has landed on laravel 5.3, and this commit allows to use
it on any compatible version.

See laravel/framework#16281 for details and use
cases.
frasmage pushed a commit to plank/laravel-mediable that referenced this pull request Nov 25, 2016
Add the ability to set a custom url for a disk.
This feature has landed on laravel 5.3, and this commit allows to use
it on any compatible version.

See laravel/framework#16281 for details and use
cases.
@IvanBernatovic
Copy link

@taylorotwell I actually need it now for the same reasons @shadoWalker89 presented you. So have no regrets, this is useful for people.

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.

None yet

4 participants