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.

Sign up for GitHub

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

[11.x] Refactor routes to dedicated file #1464

Merged
merged 4 commits into from
Jul 18, 2021
Merged

[11.x] Refactor routes to dedicated file #1464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73e76a6
Refactor routes to dedicated file
driesvints Jul 16, 2021
dace3bd
Update web.php
driesvints Jul 17, 2021
4313d53
Apply fixes from StyleCI (#1465)
driesvints Jul 17, 2021
ccfc232
formatting
taylorotwell Jul 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions routes/web.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php

use Illuminate\Support\Facades\Route;

Route::post('/token', [
'uses' => 'AccessTokenController@issueToken',
'as' => 'token',
'middleware' => 'throttle',
]);

Route::middleware(['web', 'auth'])->group(function () {
Route::post('/token/refresh', [
'uses' => 'TransientTokenController@refresh',
'as' => 'token.refresh',
]);

Route::get('/authorize', [
'uses' => 'AuthorizationController@authorize',
'as' => 'authorizations.authorize',
]);

Route::post('/authorize', [
'uses' => 'ApproveAuthorizationController@approve',
'as' => 'authorizations.approve',
]);

Route::delete('/authorize', [
'uses' => 'DenyAuthorizationController@deny',
'as' => 'authorizations.deny',
]);

Route::get('/tokens', [
'uses' => 'AuthorizedAccessTokenController@forUser',
'as' => 'tokens.index',
]);

Route::delete('/tokens/{token_id}', [
'uses' => 'AuthorizedAccessTokenController@destroy',
'as' => 'tokens.destroy',
]);

Route::get('/clients', [
'uses' => 'ClientController@forUser',
'as' => 'clients.index',
]);

Route::post('/clients', [
'uses' => 'ClientController@store',
'as' => 'clients.store',
]);

Route::put('/clients/{client_id}', [
'uses' => 'ClientController@update',
'as' => 'clients.update',
]);

Route::delete('/clients/{client_id}', [
'uses' => 'ClientController@destroy',
'as' => 'clients.destroy',
]);

Route::get('/scopes', [
'uses' => 'ScopeController@all',
'as' => 'scopes.index',
]);

Route::get('/personal-access-tokens', [
'uses' => 'PersonalAccessTokenController@forUser',
'as' => 'personal.tokens.index',
]);

Route::post('/personal-access-tokens', [
'uses' => 'PersonalAccessTokenController@store',
'as' => 'personal.tokens.store',
]);

Route::delete('/personal-access-tokens/{token_id}', [
'uses' => 'PersonalAccessTokenController@destroy',
'as' => 'personal.tokens.destroy',
]);
});
26 changes: 0 additions & 26 deletions src/Passport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use Carbon\Carbon;
use DateInterval;
use DateTimeInterface;
use Illuminate\Support\Facades\Route;
use League\OAuth2\Server\ResourceServer;
use Mockery;
use Psr\Http\Message\ServerRequestInterface;
Expand Down Expand Up @@ -157,31 +156,6 @@ public static function enableImplicitGrant()
return new static;
}

/**
* Binds the Passport routes into the controller.
*
* @param callable|null $callback
* @param array $options
* @return void
*/
public static function routes($callback = null, array $options = [])
{
$callback = $callback ?: function ($router) {
$router->all();
};

$defaultOptions = [
'prefix' => 'oauth',
'namespace' => '\Laravel\Passport\Http\Controllers',
];

$options = array_merge($defaultOptions, $options);

Route::group($options, function ($router) use ($callback) {
$callback(new RouteRegistrar($router));
});
}

/**
* Set the default scope(s). Multiple scopes may be an array or specified delimited by spaces.
*
Expand Down
77 changes: 62 additions & 15 deletions src/PassportServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Illuminate\Support\Facades\Cookie;
use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Request;
use Illuminate\Support\Facades\Route;
use Illuminate\Support\ServiceProvider;
use Laravel\Passport\Bridge\PersonalAccessGrant;
use Laravel\Passport\Bridge\RefreshTokenRepository;
Expand All @@ -34,13 +35,61 @@ class PassportServiceProvider extends ServiceProvider
*/
public function boot()
{
$this->loadViewsFrom(__DIR__.'/../resources/views', 'passport');
$this->registerRoutes();
$this->registerResources();
$this->registerMigrations();
$this->registerPublishing();
$this->registerCommands();

$this->deleteCookieOnLogout();
}

if ($this->app->runningInConsole()) {
$this->registerMigrations();
/**
* Register the Passport routes.
*
* @return void
*/
protected function registerRoutes()
{
Route::group([
Copy link
Contributor

@siarheipashkevich siarheipashkevich Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints hello, why did you not move the group declaration to web.php?

In that case you can avoid useless Route::group declaration if routes are cached (condition inside $this->loadRoutesFrom()).

And this method will look like:

protected function registerRoutes()
{
    $this->loadRoutesFrom(__DIR__.'/../routes/web.php');
}

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do this mostly in our packages. It's fine I think.

Copy link
Contributor

@siarheipashkevich siarheipashkevich Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've checked all laravel packages with the route files and think that the core team avoids using config inside route files, but I saw that you do it. And also we have duplicated conditions in that way.

For example horizon package:
https://github.com/laravel/horizon/blob/5.x/src/HorizonServiceProvider.php#L54

there is we have the condition which already exists inside the loadRoutesFrom method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siarheipashkevich this condition was added in laravel/horizon#1367 only recently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints thanks, but I think we should optimize this for avoiding extra time of code execution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints in another laravel packages we have also duplicates

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine really.

'as' => 'passport.',
'prefix' => config('passport.path', 'oauth'),
'namespace' => 'Laravel\Passport\Http\Controllers',
], function () {
$this->loadRoutesFrom(__DIR__.'/../routes/web.php');
});
}

/**
* Register the Passport resources.
*
* @return void
*/
protected function registerResources()
{
$this->loadViewsFrom(__DIR__.'/../resources/views', 'passport');
}

/**
* Register the Passport migration files.
*
* @return void
*/
protected function registerMigrations()
{
if ($this->app->runningInConsole() && Passport::$runsMigrations && ! config('passport.client_uuids')) {
$this->loadMigrationsFrom(__DIR__.'/../database/migrations');
}
}

/**
* Register the package's publishable resources.
*
* @return void
*/
protected function registerPublishing()
{
if ($this->app->runningInConsole()) {
$this->publishes([
__DIR__.'/../database/migrations' => database_path('migrations'),
], 'passport-migrations');
Expand All @@ -52,26 +101,24 @@ public function boot()
$this->publishes([
__DIR__.'/../config/passport.php' => config_path('passport.php'),
], 'passport-config');

$this->commands([
Console\InstallCommand::class,
Console\ClientCommand::class,
Console\HashCommand::class,
Console\KeysCommand::class,
Console\PurgeCommand::class,
]);
}
}

/**
* Register Passport's migration files.
* Register the Passport Artisan commands.
*
* @return void
*/
protected function registerMigrations()
protected function registerCommands()
{
if (Passport::$runsMigrations && ! config('passport.client_uuids')) {
$this->loadMigrationsFrom(__DIR__.'/../database/migrations');
if ($this->app->runningInConsole()) {
$this->commands([
Console\InstallCommand::class,
Console\ClientCommand::class,
Console\HashCommand::class,
Console\KeysCommand::class,
Console\PurgeCommand::class,
]);
}
}

Expand Down
165 changes: 0 additions & 165 deletions src/RouteRegistrar.php
View file
Open in desktop

This file was deleted.

Loading