-
Notifications
You must be signed in to change notification settings - Fork 1
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
[main] Update framework to version 10 #106
Conversation
database/factories/UserFactory.php
Outdated
'name' => fake()->name(), | ||
'email' => fake()->unique()->safeEmail(), | ||
'email_verified_at' => now(), | ||
'password' => '$2y$10$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi', // password |
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.
why not use bcrypt like before?
| | ||
*/ | ||
|
||
$app = require_once __DIR__.'/../bootstrap/app.php'; | ||
require __DIR__.'/../vendor/autoload.php'; |
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.
I really hope you know what you're doing in this file 😅
@@ -13,6 +13,6 @@ | |||
| | |||
*/ | |||
|
|||
Route::middleware('auth:api')->get('/user', function (Request $request) { | |||
Route::middleware('auth:sanctum')->get('/user', function (Request $request) { |
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.
Is this correct? Why is it changed from api to sanctum?
| | ||
*/ | ||
|
||
Route::get('/', [HomeController::class, 'index'])->name('home'); |
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.
Here too I hope you know what you're doing haha. Hopefully all went well
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.
See comments
app/Exceptions/Handler.php
Outdated
{ | ||
return parent::render($request, $exception); | ||
} | ||
$this->reportable(function (Throwable $e) { |
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.
This was updated from Laravel itself. See commit here:
laravel/laravel@4931af1
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.
Welp I don't think I added much with this review but I hope it's still a bit useful :3
{ | ||
$this->registerPolicies(); |
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.
Is this line of code not necessary anymore?
Route::get('login/{token}', 'LoginController@loginByToken'); | ||
Route::match(['get', 'post'], 'login', 'LoginController@login')->name('login'); | ||
Route::post('logout', 'LoginController@logout')->name('logout'); | ||
Route::get('login/{token}', [LoginController::class, 'loginByToken']); |
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.
Maybe this route could also have a name for consistencyy
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 reason I didn't add a name to this one in the past, is because with a name you allow to redirect/link to it through code. So in a way I tried to enforce here that you can only do it by manually putting in the URL in the browser
|
||
'max_age' => 0, | ||
|
||
'supports_credentials' => false, |
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.
Maybe this should be set to true at some point in the future if authentication is necessary for the api/* routes?
require sail
No description provided.