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

Support Laravel 11 #323

Merged
merged 24 commits into from
Feb 14, 2024
Merged

Support Laravel 11 #323

merged 24 commits into from
Feb 14, 2024

Conversation

luanfreitasdev
Copy link
Contributor

This PR adds the possibility of supporting Laravel 11 in the log viewer. I have some doubts regarding support so I opened PR with tests failing.

Is there a possibility that a change supported by Laravel 11 breaks the others (9, 10) and PHP 8.0, 8.1?

This Pull Request can be closed at any time by the log viewer team if it is not a priority or if it can be different or can also be worked on together

@luanfreitasdev
Copy link
Contributor Author

I believe the main change in this project was the break in structure between carbon v2 and v3

*Some solutions I found:

  • Require carbon v2 (this may fail if someone tries to use carbon v3 in Laravel 11)
  • Change the return type (removes the tz after parseDatetime and calls it inside the method).

To compare:

Version 2:
https://github.com/briannesbitt/Carbon/blob/57fbbf88ce332f6da4e5aa1ea7524ef1caebb9e4/src/Carbon/CarbonInterface.php#L4958

Version 3
https://github.com/briannesbitt/Carbon/blob/1bf13e0f69f405c1eb8e59783f55853a6710da7a/src/Carbon/CarbonInterface.php#L4555

@luanfreitasdev luanfreitasdev marked this pull request as ready for review February 10, 2024 16:54
src/Logs/HttpAccessLog.php Outdated Show resolved Hide resolved
.github/workflows/run-tests.yml Outdated Show resolved Hide resolved
.github/workflows/run-tests.yml Outdated Show resolved Hide resolved
@arukompas
Copy link
Contributor

hey @luanfreitasdev thanks for the PR! Would be great to have Log Viewer ready before L11 release.

First of all, I think you may have misunderstood the intention of the log-viewer.timezone configuration value, which I've explained in a comment above. With that in mind, I think the changes related to how the timezone is set should be reverted.

Second, there were a few changes to the GitHub Actions workflow that I think were unnecessary :)

And lastly, is there a resource on Laravel 11 and its changes/requirements already that I can double-check before merging the PR?

@luanfreitasdev
Copy link
Contributor Author

@arukompas . I added the main corrections, removing the time zone when creating the Carbon object and adding it later using setTimezone.

Github's actions have been rolled back.

I didn't understand the last point. Did you mean do we have anything else to look forward to before Laravel 11?

@luanfreitasdev luanfreitasdev marked this pull request as draft February 13, 2024 21:42
@luanfreitasdev luanfreitasdev marked this pull request as ready for review February 13, 2024 21:49
src/Logs/LaravelLog.php Outdated Show resolved Hide resolved
src/Logs/Log.php Outdated Show resolved Hide resolved
tests/Unit/LogTest.php Outdated Show resolved Hide resolved
@arukompas
Copy link
Contributor

@luanfreitasdev great, thanks for the changes! Missed a few, noted above.

As for the last part of what I meant. I don't know if there will be any breaking changes in Laravel 11. Are you aware of any? If so, where can I find them? If not then it's fine :)

@luanfreitasdev
Copy link
Contributor Author

Hello, @arukompas !. I believe there is nothing significant in the case of the logs

@arukompas
Copy link
Contributor

hey @luanfreitasdev , just one more issue mentioned above in the un-resolved comment :) We're almost there!

@luanfreitasdev
Copy link
Contributor Author

Good reviewer 🔥

@arukompas arukompas merged commit 5092d53 into opcodesio:main Feb 14, 2024
1 check passed
@arukompas
Copy link
Contributor

@luanfreitasdev big thanks, v3.4.0 tagged with support for Laravel 11! 🎉

All tests are passing everywhere.

Screenshot 2024-02-14 at 17 13 46

@luanfreitasdev luanfreitasdev deleted the feat/laravel11 branch February 14, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants