Laravel 10 Upgrade #730
Laravel 10 Upgrade #730
Conversation
deer-wmde
force-pushed
the
de/laravel-10
branch
from
031a055 to
c0ecdb3
Compare
Composer version 2.6.5 2023-10-06 10:11:52
deer-wmde
force-pushed
the
de/laravel-10
branch
from
c0ecdb3 to
057906a
Compare
m90
left a comment
m90
left a comment
There was a problem hiding this comment.
This looks pretty solid already. I left a few questions inline, nothing blocking.
| * Example: | ||
| * | ||
| * php artisan job:dispatchNow CirrusSearch\\ForceSearchIndex id 1 0 1000 | ||
| * php artisan job:dispatchSync CirrusSearch\\ForceSearchIndex id 1 0 1000 |
There was a problem hiding this comment.
I wonder if we want dispatch instead. If I understand it correctly dispatchSync will not use the existing queue setup, but somehow bypass this. Right now, I would think we want this to run together with the other jobs instead.
But maybe this is also something we should look into with https://phabricator.wikimedia.org/T342866
There was a problem hiding this comment.
I just realized this console command is completely separate from the framework anyway... https://github.com/mxl/laravel-job/blob/master/src/Commands/DispatchNow.php
But still I think you are right, dispatch is probably a better choice most of the time. I'll change the comments accordingly
There was a problem hiding this comment.
There are some more critical instances where I'd leave it though, MediawikiUpdate.php for example https://github.com/wbstack/api/blob/e59eda7de24f1baccbb7a02c285f684eebe93ecd/app/Jobs/MediawikiUpdate.php
| * @var array<string, string> | ||
| */ | ||
| protected $casts = [ | ||
| 'email_verified_at' => 'datetime', |
There was a problem hiding this comment.
Is this a drive by improvement or something we have to when using Laravel 10?
There was a problem hiding this comment.
Can't tell if it's strictly necessary but looked like current best practice to me in regards to https://laravel.com/docs/10.x/upgrade#model-dates-property
I think this particular change was copied from the corresponding section of app/Models/User.php
deer-wmde
force-pushed
the
de/laravel-10
branch
from
1c1e4dd to
f9b6611
Compare
deer-wmde
force-pushed
the
de/laravel-10
branch
from
f9b6611 to
35ef59b
Compare
https://phabricator.wikimedia.org/T341797
note:
dispatchNowhas been replaced bydispatchSync(but only in the code, not for the console command! https://github.com/mxl/laravel-job/blob/master/src/Commands/DispatchNow.php)