tenancy / multi-tenant

Run multiple websites using the same Laravel installation while keeping tenant specific data separated for fully independent multi-domain setups, previously github.com/hyn/multi-tenant
https://tenancy.dev
MIT License
2.56k stars 394 forks source link

Fixed code in comment #907

Closed Bibendus83 closed 3 years ago

Bibendus83 commented 4 years ago

I had issues because I added this code in my routes and it didn't work. Checking the documentation I noticed the difference https://tenancy.dev/docs/hyn/5.4/structure

fletch3555 commented 4 years ago

The current code is a class reference, and evaluates at runtime to the string shown in the docs. You're correct that they don't match exactly, but they are functionally equivalent.

Also, the code in this PR is invalid and will throw errors. You've changed it from a class reference, to the class name, so the syntax is invalid. You would need to add quotes around that string if you remove ::class.

Either way, I don't see a need to change this. If you're having issues with your routes, please post in our discord chat and we'll gladly attempt to help you solve them.

Bibendus83 commented 4 years ago

Oh yes sorry, I forgot the quotes! For clarity I would modify it to match the documentation, that causes confusion for no reason.

I know using the class reference should be the same, however I get an exception when I use :class instead of writing the class name as string. UnexpectedValueException Invalid route action: [App\Http\Controllers\Hyn\Tenancy\Controllers\MediaController]

fletch3555 commented 4 years ago

Ah, I see the problem. It's currently a "relative" class reference. The fix is to add a slash at the front (and leave ::class). This makes it an "absolute" class reference. Relative means it searches for the class using the current package (App\Http\Controllers in this case) as a prefix.

Bibendus83 commented 4 years ago

Mmmm you should be right, in fact I now notice the App\Http\Controllers\ appended before the action class. However I was already using the front slash and I still get the same error

Route::get('/media/{path}', \Hyn\Tenancy\Controllers\MediaController::class)
    ->where('path', '.+')
    ->name('tenant.media');

Invalid route action: [App\Http\Controllers\Hyn\Tenancy\Controllers\MediaController].

Maybe it's a cache problem but if I change it to string it works.

fletch3555 commented 4 years ago

Try changing it just MediaController::class and move the rest to a use statement at the top of the file. I see no reason it shouldn't work how you have it. Perhaps your routes are cached, so you could try clearing that as well (artisan route:clear I think...?)

Bibendus83 commented 4 years ago

I already called a php artisan route:clear but nothing changed. Tried the use statement at the beginning of the file but same error.

use Hyn\Tenancy\Controllers\MediaController;

Route::get('/media/{path}', MediaController::class)
    ->where('path', '.+')
    ->name('tenant.media');

That's odd...

fletch3555 commented 4 years ago

Wait, I think I know what it is..... you're working in routes/web.php, right?

Bibendus83 commented 4 years ago

yes

fletch3555 commented 4 years ago

That App\Http\Controller prefix is added by the mechanism that loads web.php routes, RouteServiceProvider. The string may be the only way to make this work, but I'm not certain why at the moment

drbyte commented 4 years ago

The core RouteServiceProvider injects the namespace. (You can see it where it calls the routes/web.php file). https://github.com/laravel/laravel/blob/166abfa35c535f4572d5971a99aec45cc8c63ff6/app/Providers/RouteServiceProvider.php#L59-L64

I see 4 options:

ArlonAntonius commented 3 years ago

Don't see a reason to merge this really. Everything seems fine as is and it's upto the setup of the developer on whether this works in one go or not.