-
-
Notifications
You must be signed in to change notification settings - Fork 444
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
[4.x] This adds support for tenancy aware Storage::url() method #689
Conversation
Thanks a lot for the PR! The code looks very high quality. I'll review & merge this in a few weeks when I start work on v4 and will be making all changes at once. Thanks again! |
Thanks :-) Tomorrow I'll try to write a better description of all changes. |
@stancl any plans of merging this? idk but seems like tests failed |
Hi @aneeskhan47 , tests failed not because of code, but because of an error in building one of the docker containers in workflow. Otherwise, all tests should pass. You can test it yourself by cloning this pull request (e.g. to Gitpod) and run provided docker (see CONTRIBUTING.md) If you want to use this in v3 you can extract my changes to custom TenancyBootstrapper. I would help you more, but right now I'm at the hospital recovering from an accident and I'm limited to my phone screen. |
@dontfreakout first of all thank you for commenting and thank you for the pr as well. Yes I'm thinking to use your pr code to my custom tenancy boostrapper And I hope @stancl will merge this to currentl v3 or upcoming v4 🤗 |
@aneeskhan47 you are very welcome. It should be pretty straightforward. This PR is extracted (with some modifications) from the bootstrapper I'm using in v3. If all goes well, next week I'll be back home and can create a gist, if it helps. |
@dontfreakout i have extracted your pr code to mine. i have added my own FilesystemTenancyBootstrapper (with your code) + the link command to my App\Console\Commands directory and modified the tenancy config to use my FilesystemTenancyBootstrapper plus added the url_override option. i also ran the tenant:link command and it succesfully linked, but now the global_asset helper got kinda broken: |
@dontfreakout actually the reason of the global asset broke is that in tenancy original TenancyServiceProvider.php in /src the globalUrl was bounding to the for the time being i have copied your PR code and i hope this PR to get merged @stancl so then i can use the package normally :) Thanks @dontfreakout and @stancl |
@aneeskhan47 did you disabled original FilesystemBootrapper in the config? I can't do much right now. The phone screen is not very comfortable for anything about code 😒 About the symlinks... Can you check if created tenant dir in the public dir is a symlink or just a plain directory? |
@aneeskhan47 you are faster than me 🙂 |
Couldn't find the button to re-run the build, so I pushed a small change. The Docker issue was probably a random thing that'll be fixed by re-running the build. Thanks for the help here @dontfreakout |
Ok, so @aneeskhan47 were right about the bug in the link command (at least for L6). For the link command, I'm extending Laravel StorageLinkCommand, but L6 can create links only from "public/storage" to "storage/app/public". I'll fix this once I get to my PC (next week, I hope). |
@dontfreakout Hi, i hope you are doing well. when you will make the fix ? if you are unable for some reason tell me the changes and i'll add them 😄 |
…rageLink tests, added RemoveStorageSymlinks Job, added Storage Jobs to TenancyServiceProvider stub, renamed misleading config example.
@aneeskhan47 Hi, I'm back home after almost three weeks in hospital, so I'm doing definitely better. Thanks for asking :-) The fix is committed. I've also added a few more features: Events, RemoveSymlink Job. And added Jobs to TenancyServiceProvider stub (to create and remove symlink when created/deleted Tenancy). @stancl to manually trigger a workflow, the workflow must be configured to run on the |
This adds a lot of new stuff, so I'll add it in v4, but I'll also have to make some changes when I'll be merging this. As I mentioned in #654, Laravel 9 uses flysystem 2, which forces me to rewrite how the FilesystemTenancyBootstrapper works. So once I have the v4 filesystem bootstrapper done, I'll update the code of this and merge 👍🏻 |
Sorry, if i upgrade package to v3.5 can i use php artisan tenants:link ? |
It will be available in v4. |
FWIW the filesystem logic (that uses adapters) will need to be adjusted after #802 is merged |
Add this code to your
/*
* Use this to support Storage url method on local driver disks.
* You should create a symbolic link which points to the public directory using command: artisan tenants:link
* Then you can use tenant aware Storage url: Storage::disk('public')->url('file.jpg')
*/
'url_override' => [
// The array key is local disk (must exist in root_override) and value is public directory (%tenant_id% will be replaced with actual tenant id).
'public' => 'public-%tenant_id%',
],
\Illuminate\Support\Facades\URL::macro('tenantFile', function(?string $file = '') {
if (is_null($file)) {
return null;
}
// file is the url information, which is returned directly. No tenant domain name splicing.
if (str_contains($file, '://')) {
return $file;
}
$prefix = str_replace(
'%tenant_id%',
tenant()->getKey(),
config('tenancy.filesystem.url_override.public', 'public-%tenant_id%')
);
return url($prefix.'/'.$file);
});
Events\TenantCreated::class => [
JobPipeline::make([
Jobs\CreateDatabase::class,
Jobs\MigrateDatabase::class,
Jobs\SeedDatabase::class,
// Your own jobs to prepare the tenant.
// Provision API keys, create S3 buckets, anything you want!
])->send(function (Events\TenantCreated $event) {
$tenant = $event->tenant;
$target = base_path(sprintf("storage/%s%s/app/public",
config('tenancy.filesystem.suffix_base'),
$tenant->id));
$link = str_replace('%tenant_id%', $tenant->id, config('tenancy.filesystem.url_override.public', 'public-%tenant_id%'));
chdir(public_path());
\Illuminate\Support\Facades\File::ensureDirectoryExists(dirname($target));
\Illuminate\Support\Facades\File::link($target, $link);
return $event->tenant;
})->shouldBeQueued(false), // `false` by default, but you probably want to make this `true` for production.
],
Events\TenantDeleted::class => [
JobPipeline::make([
Jobs\DeleteDatabase::class,
])->send(function (Events\TenantDeleted $event) {
$tenant = $event->tenant;
$target = base_path(sprintf("storage/%s%s/app/public",
config('tenancy.filesystem.suffix_base'),
$tenant->id));
$link = str_replace('%tenant_id%', $tenant->id, config('tenancy.filesystem.url_override.public', 'public-%tenant_id%'));
chdir(public_path());
\Illuminate\Support\Facades\File::delete($link);
\Illuminate\Support\Facades\File::deleteDirectory(dirname($target));
return $event->tenant;
})->shouldBeQueued(false), // `false` by default, but you probably want to make this `true` for production.
],
$path = str_replace('public/', '', $path);
\Illuminate\Support\Facades\URL::tenantFile($path); |
Seems like I didn't resolve the conflicts correctly. Adding a note to add back some of the removed logic from here accbf5b#diff-a7131a9582938c1b642f3d517a9064cbacb7b83d75fb52fe2c6e52dbca26a584 |
$config->set('url', $this->originalPaths['disks']['url']); | ||
$url = $this->originalPaths['disks.url.' . $disk] ?? null; | ||
|
||
if ($this->getDiskConfig($disk)['driver'] === 'local' && ! is_null($url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getDiskConfig()
method doesn't make much sense. If you're returning null, but using ['driver']
on the return value, that will return in exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, there should be better error handling (would the exception be different from the ones that pieces of the original code would throw?). But if there's no disk with the passed name, the method should return null, that makes sense to me.
The conflict resolution is a bit of a mess now. I'm thinking about forcibly reverting to the point before I merged master into this branch, and you'd try to resolve the conflicts there from scratch. Now it's a mix of my changes with your changes, and your changes still need to be modified (eg the review above). And there are more conflicts to resolve in other files. Perhaps you could create a branch based on I'd like to avoid having excessive back & forth about the conflict resolution in this specific thread. |
I'll close this now in favor of #909 |
* This adds support for tenancy aware Storage::url() method * Trigger CI build * Fixed Link command for Laravel v6, added StorageLink Events, more StorageLink tests, added RemoveStorageSymlinks Job, added Storage Jobs to TenancyServiceProvider stub, renamed misleading config example. * Fix typo * Fix code style (php-cs-fixer) * Update config comments * Format code in Link command, make writing more concise * Change "symLinks" to "symlinks" * Refactor Link command * Fix test name typo * Test fetching files using the public URL * Extract Link command logic into actions * Fix code style (php-cs-fixer) * Check if closure is null in CreateStorageSymlinksAction * Stop using command terminology in CreateStorageSymlinksAction * Separate the Storage::url() test cases * Update url_override comments * Remove afterLink closures, add types, move actions, add usage explanation to the symlink trait * Fix code style (php-cs-fixer) * Update public storage URL test * Fix issue with using str() * Improve url_override comment, add todos * add todo comment * fix docblock style * Add link command tests back * Add types to $tenants in the action handle() methods * Fix typo, update variable name formatting * Add tests for the symlink actions * Change possibleTenantSymlinks not to prefix the paths twice while tenancy is initialized * Fix code style (php-cs-fixer) * Stop testing storage directory existence in symlink test * Don't specify full namespace for Tenant model annotation * Don't specify full namespace in ActionTest * Remove "change to DI" todo * Remove possibleTenantSymlinks return annotation * Remove symlink-related jobs, instantiate and use actions * Revert "Remove symlink-related jobs, instantiate and use actions" This reverts commit 547440c. * Add a comment line about the possible tenant symlinks * Correct storagePath and publicPath variables * Revert "Correct storagePath and publicPath variables" This reverts commit e3aa8e2. * add a todo Co-authored-by: Martin Vlcek <martin@dontfreakout.eu> Co-authored-by: lukinovec <lukinovec@gmail.com> Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
* This adds support for tenancy aware Storage::url() method * Trigger CI build * Fixed Link command for Laravel v6, added StorageLink Events, more StorageLink tests, added RemoveStorageSymlinks Job, added Storage Jobs to TenancyServiceProvider stub, renamed misleading config example. * Fix typo * Fix code style (php-cs-fixer) * Update config comments * Format code in Link command, make writing more concise * Change "symLinks" to "symlinks" * Refactor Link command * Fix test name typo * Test fetching files using the public URL * Extract Link command logic into actions * Fix code style (php-cs-fixer) * Check if closure is null in CreateStorageSymlinksAction * Stop using command terminology in CreateStorageSymlinksAction * Separate the Storage::url() test cases * Update url_override comments * Remove afterLink closures, add types, move actions, add usage explanation to the symlink trait * Fix code style (php-cs-fixer) * Update public storage URL test * Fix issue with using str() * Improve url_override comment, add todos * add todo comment * fix docblock style * Add link command tests back * Add types to $tenants in the action handle() methods * Fix typo, update variable name formatting * Add tests for the symlink actions * Change possibleTenantSymlinks not to prefix the paths twice while tenancy is initialized * Fix code style (php-cs-fixer) * Stop testing storage directory existence in symlink test * Don't specify full namespace for Tenant model annotation * Don't specify full namespace in ActionTest * Remove "change to DI" todo * Remove possibleTenantSymlinks return annotation * Remove symlink-related jobs, instantiate and use actions * Revert "Remove symlink-related jobs, instantiate and use actions" This reverts commit 547440c. * Add a comment line about the possible tenant symlinks * Correct storagePath and publicPath variables * Revert "Correct storagePath and publicPath variables" This reverts commit e3aa8e2. * add a todo Co-authored-by: Martin Vlcek <martin@dontfreakout.eu> Co-authored-by: lukinovec <lukinovec@gmail.com> Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
* Add readied tenants Add config for readied tenants Add `create` and `clear` command Add Readied scope and static functions Add tests * Fix initialize function name * Add readied events * Fix readied column cast * Laravel 6 compatible * Add readied scope tests * Rename config from include_in_scope to include_in_queries * Change terminology to pending * Update CreatePendingTenants.php * Laravel 6 compatible * Update CreatePendingTenants.php * runForMultiple can scope pending tenants * Fix issues * Code and comment style improvements * Change 'tenant' to 'tenants' in command signature * Fix code style (php-cs-fixer) * Rename variables in CreatePendingTenants * Remove withPending from runForMultiple * Update tenants option trait * Update command that use tenants * Fix code style (php-cs-fixer) * Improve getTenants condition * Update config comments * Minor config comment corrections * Grammar fix * Update comments and naming * Correct comments * Improve writing * Remove pending tenant clearing time constraints * Allow using only one time constraint for clearing the pending tenants * phpunit to pest * Fix code style (php-cs-fixer) * Fix code style (php-cs-fixer) * [4.x] Optionally delete storage after tenant deletion (#938) * Add test for deleting storage after tenant deletion * Save `storage_path()` in a variable after initializing tenant in test Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com> * Add DeleteTenantStorage listener * Update test name * Remove storage deletion config key * Remove tenant storage deletion events * Move tenant storage deletion to the DeletingTenant event Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com> * [4.x] Finish incomplete and missing tests (#947) * complete test sqlite manager customize path * complete test seed command works * complete uniqe exists test * Update SingleDatabaseTenancyTest.php * refactor the ternary into if condition * custom path * simplify if condition * random dir name * Update SingleDatabaseTenancyTest.php * Update CommandsTest.php * prefix random DB name with custom_ Co-authored-by: Samuel Štancl <samuel@archte.ch> * [4.x] Add batch tenancy queue bootstrapper (#874) * exclude master from CI * Add batch tenancy queue bootstrapper * add test case * skip tests for old versions * variable docblocks * use Laravel's connection getter and setter * convert test to pest * bottom space * singleton regis in TestCase * Update src/Bootstrappers/BatchTenancyBootstrapper.php Co-authored-by: Samuel Štancl <samuel@archte.ch> * convert batch class resolution to property level * enabled BatchTenancyBootstrapper by default * typehint DatabaseBatchRepository * refactore name * DI DB manager * typehint * Update config.php * use initialize() twice without end()ing tenancy to assert that previousConnection logic works correctly Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com> Co-authored-by: Abrar Ahmad <abrar.dev99@gmail.com> Co-authored-by: Samuel Štancl <samuel@archte.ch> * [4.x] Storage::url() support (modified #689) (#909) * This adds support for tenancy aware Storage::url() method * Trigger CI build * Fixed Link command for Laravel v6, added StorageLink Events, more StorageLink tests, added RemoveStorageSymlinks Job, added Storage Jobs to TenancyServiceProvider stub, renamed misleading config example. * Fix typo * Fix code style (php-cs-fixer) * Update config comments * Format code in Link command, make writing more concise * Change "symLinks" to "symlinks" * Refactor Link command * Fix test name typo * Test fetching files using the public URL * Extract Link command logic into actions * Fix code style (php-cs-fixer) * Check if closure is null in CreateStorageSymlinksAction * Stop using command terminology in CreateStorageSymlinksAction * Separate the Storage::url() test cases * Update url_override comments * Remove afterLink closures, add types, move actions, add usage explanation to the symlink trait * Fix code style (php-cs-fixer) * Update public storage URL test * Fix issue with using str() * Improve url_override comment, add todos * add todo comment * fix docblock style * Add link command tests back * Add types to $tenants in the action handle() methods * Fix typo, update variable name formatting * Add tests for the symlink actions * Change possibleTenantSymlinks not to prefix the paths twice while tenancy is initialized * Fix code style (php-cs-fixer) * Stop testing storage directory existence in symlink test * Don't specify full namespace for Tenant model annotation * Don't specify full namespace in ActionTest * Remove "change to DI" todo * Remove possibleTenantSymlinks return annotation * Remove symlink-related jobs, instantiate and use actions * Revert "Remove symlink-related jobs, instantiate and use actions" This reverts commit 547440c. * Add a comment line about the possible tenant symlinks * Correct storagePath and publicPath variables * Revert "Correct storagePath and publicPath variables" This reverts commit e3aa8e2. * add a todo Co-authored-by: Martin Vlcek <martin@dontfreakout.eu> Co-authored-by: lukinovec <lukinovec@gmail.com> Co-authored-by: PHP CS Fixer <phpcsfixer@example.com> * Use HasTenantOptions in Link * Correct the tenant order in Run command * Fix code style (php-cs-fixer) * Fix formatting issue * Add missing imports * Fix code style (php-cs-fixer) * Use HasTenantOptions instead of the old trait name in Up/Down commands * Fix test name typo * Remove redundant passing of $withPending to runForMultiple in TenantCollection's runForEach * Make `with-pending` default to `config('tenancy.pending.include_in_queries')` in HasTenantOptions * Make `createPending()` return the created tenant * Fix code style (php-cs-fixer) * Remove tenant ordering * Fix code style (php-cs-fixer) * Remove duplicate tenancy bootstrappers config setting * Add and use getWithPendingOption method * Fix code style (php-cs-fixer) * Add optionNotPassedValue property * Test using --with-pending and the include_in_queries config value * Make with-pending VALUE_NONE * use plural in test names * fix test names * add pullPendingTenantFromPool * Add docblock type * Import commands * Fix code style (php-cs-fixer) * Move pending tenant tests to a more appropriate file * Delete queuetest from gitignore * Delete queuetest file * Add queuetest to gitignore * Rename pullPendingTenant to pullPending and don't pass bool to that method * Add a test that checks if pulling a pending tenant removes it from the pool * bump stancl/virtualcolumn to ^1.3 * Update pending tenant pulling test * Dynamically get columns for pending queries * Dynamically get virtual column name in ClearPendingTenants * Fix ClearPendingTenants bug * Make test name more accurate * Update test name * add a todo * Update include in queries test name * Remove `Tenant::query()->delete()` from pending tenant check test * Rename the pending tenant check test name * Update HasPending.php * fix all() call * code style * all() -> get() * Remove redundant `Tenant::all()` call Co-authored-by: j.stein <joristein@gmail.com> Co-authored-by: lukinovec <lukinovec@gmail.com> Co-authored-by: PHP CS Fixer <phpcsfixer@example.com> Co-authored-by: Abrar Ahmad <abrar.dev99@gmail.com> Co-authored-by: Riley19280 <rileyaven88@gmail.com> Co-authored-by: Martin Vlcek <martin@dontfreakout.eu>
This fixes #78 and #201 and enables using
Storage::url('file.jpg')
with local driver in tenants (see Laravel docs).With this, you can use for example Spatie media library and local disk without any modifications.
You only need to create symlinks in the public directory which points to the tenants' disks root.
For creating symlinks you can use the new command
tenants:link