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

Code health updates #151

Merged
merged 7 commits into from
Dec 30, 2024
Merged

Code health updates #151

merged 7 commits into from
Dec 30, 2024

Conversation

xHeaven
Copy link
Contributor

@xHeaven xHeaven commented Dec 28, 2024

  • Updated old str_* methods to newer PHP 8 str_* methods
  • Marked implicitly nullable parameters as explicitly nullable, as implicit nullability is deprecated since PHP 8.4
  • Fixed a typo
  • Added missing typehints wherever it made sense
  • Replaced tap() with defer() in a test - tap() is deprecated, it is essentially an alias for defer()

@xHeaven
Copy link
Contributor Author

xHeaven commented Dec 30, 2024

Tests are failing due to this PR: #150

Should we just remove the --disk option from the call?

@freekmurze
Copy link
Member

I've remove that faulty test on main. Could you rebase to check if the tests now pass for this PR?

@freekmurze
Copy link
Member

We still have some failing tests it seems

@xHeaven
Copy link
Contributor Author

xHeaven commented Dec 30, 2024

We still have some failing tests it seems

Interesting, apparently tap() is deprecated since a specific version of Pest but doesn't exist yet in older versions.
Should I revert 7a3dc0d (#151) or should we add some kind of conditional based on some criteria (whether defer() exists or not perhaps)?

@freekmurze
Copy link
Member

I'd be ok with a conditional. But I'd also be ok with just dropping support for older Laravel / Pest / PHP versions to fix this.

@xHeaven
Copy link
Contributor Author

xHeaven commented Dec 30, 2024

I'd be ok with a conditional. But I'd also be ok with just dropping support for older Laravel / Pest / PHP versions to fix this.

What are the lowest versions that you want to support?
With "pestphp/pest-plugin-laravel": "^1.3|^2.3" changed to "pestphp/pest-plugin-laravel": "^2.4" I think anything above PHP 8.1 and Laravel 10 should be fine.

Do you want to consider dropping PHP <8.1 support as well as Laravel <10 support?

@freekmurze
Copy link
Member

Do you want to consider dropping PHP <8.1 support as well as Laravel <10 support?

I'm pretty aggressive with dropping older versions, so this is certainly ok 👍

@xHeaven
Copy link
Contributor Author

xHeaven commented Dec 30, 2024

Do you want to consider dropping PHP <8.1 support as well as Laravel <10 support?

I'm pretty aggressive with dropping older versions, so this is certainly ok 👍

Glad to hear, I like to keep things fresh as well. Working on it, I will push a commit in a few minutes.

@xHeaven
Copy link
Contributor Author

xHeaven commented Dec 30, 2024

Looks pretty green to me, prefer-lowest tests got a few deprecation warnings with PHP 8.4 (Laravel Ray, Log facade being old in L10), but that's it.

@freekmurze freekmurze merged commit dcb5abd into spatie:main Dec 30, 2024
14 checks passed
@freekmurze
Copy link
Member

Perfect, thanks!

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.

2 participants