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.

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

Update Laravel to version 9 #718

Merged
merged 11 commits into from Jan 23, 2024
Merged

Update Laravel to version 9 #718

merged 11 commits into from Jan 23, 2024

Conversation

m90
Copy link
Contributor

@m90 m90 commented Jan 10, 2024

@m90 m90 force-pushed the fr/laravel-9 branch 7 times, most recently from f5b3548 to e4085da Compare January 17, 2024 15:41
@m90 m90 marked this pull request as ready for review January 17, 2024 15:51
@@ -107,6 +107,9 @@ public function handle( DatabaseManager $manager )
$replacedCount = 0;
$tableWithoutPrefix = str_replace($wikiDB->prefix . '_', '', $table, $replacedCount );
if ($replacedCount !== 1) {
/**
* @psalm-suppress InvalidCast
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really tried hard to understand these warnings but I could not make any sense of them so I suppressed them for now. If anyone has a hunch why they might show up, please lmk.

@@ -34,21 +34,21 @@ public function getBatches(Request $request): \Illuminate\Http\Response

public function markBatchesDone(Request $request): \Illuminate\Http\Response
{
$batches = (array) $request->json()->get('batches');
$batches = (array) $request->json()->all('batches');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong usage before and worked mostly by accident. Now, Symfony will throw an error on the wrong usage, so I was able to fix it.

@deer-wmde
Copy link
Contributor

  1. minor renaming was missed I think, in the upgrade guide it is mentioned that
    FILESYSTEM_DRIVER was renamed to FILESYSTEM_DISK (/config/filesystems.php)

  2. I think we can remove this line now (see auth_mode in the upgrade guide): https://github.com/wbstack/api/blob/main/config/mail.php#L45

  3. I'm a bit worried about the 30 second default http timeout that was introduced and if we depend on the previous behaviour with no timeout https://laravel.com/docs/9.x/upgrade#http-client-default-timeout

  4. Also I wonder if we should keep certain files in sync that change https://laravel.com/docs/9.x/upgrade#miscellaneous (and laravel/laravel@8.x...9.x). The guide says that many changes are not required though.

@m90
Copy link
Contributor Author

m90 commented Jan 22, 2024

minor renaming was missed I think, in the upgrade guide it is mentioned that
FILESYSTEM_DRIVER was renamed to FILESYSTEM_DISK (/config/filesystems.php)

I was mostly thinking if someone set it previously, they'd also use the old key, so we should keep it, but it seems noone ever sets it. In which case renaming makes sense I guess.

I think we can remove this line now (see auth_mode in the upgrade guide): https://github.com/wbstack/api/blob/main/config/mail.php#L45

Good catch.

I'm a bit worried about the 30 second default http timeout that was introduced and if we depend on the previous behaviour with no timeout https://laravel.com/docs/9.x/upgrade#http-client-default-timeout

I think this is benefitial actually. We rarely use the Laravel Http client anyways, and in the cases we do, If we have anything the API calls and a single call takes longer than 30s, we might wanna know about it by it failing.

Also I wonder if we should keep certain files in sync that change https://laravel.com/docs/9.x/upgrade#miscellaneous (and laravel/laravel@8.x...9.x). The guide says that many changes are not required though.

I read that comment in the docs as well and wasn't entirely sure what it meant tbh. Do you have an actionable change as an example that would be coming from this remark?

@m90
Copy link
Contributor Author

m90 commented Jan 23, 2024

Also I wonder if we should keep certain files in sync that change https://laravel.com/docs/9.x/upgrade#miscellaneous (and laravel/laravel@8.x...9.x). The guide says that many changes are not required though.

I didn't understand this repo was an application skeleton. I thought it contained the framework itself and was puzzled as why I would need to read all commit messages ever. I'll have a look at the changeset if I can find anything that might be helpful for us.

@deer-wmde
Copy link
Contributor

I think this is benefitial actually. We rarely use the Laravel Http client anyways, and in the cases we do, If we have anything the API calls and a single call takes longer than 30s, we might wanna know about it by it failing.

That makes sense, thanks.

Also thanks for looking into the skeleton changes - I haven't done an laravel upgrade yet but now it looks pretty good to me!

@m90 m90 merged commit b0afed8 into main Jan 23, 2024
5 checks passed
@m90 m90 deleted the fr/laravel-9 branch January 23, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants