search

spatie / laravel-query-builder

Easily build Eloquent queries from API requests
https://spatie.be/docs/laravel-query-builder
MIT License
4.06k stars 397 forks source link

PHP 8.1 Support #702

Closed Medalink closed 2 years ago

Medalink commented 2 years ago

Trying out this package on PHP 8.1 fails with During inheritance of ArrayAccess: Uncaught ErrorException: Return type of Spatie\QueryBuilder\QueryBuilder::offsetSet($offset, $value) should either be compatible with ArrayAccess::offsetSet(mixed $offset, mixed $value): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /app/vendor/spatie/laravel-query-builder/src/QueryBuilder.php:168

This PR addresses the issue by adding #[\ReturnTypeWillChange] to the offset methods.

I also ran into an issue with null being passed to explode: explode(): Passing null to parameter #2 ($string) of type string is deprecated {"exception":"[object] (ErrorException(code: 0): explode(): Passing null to parameter #2 ($string) of type string is deprecated at /app/vendor/spatie/laravel-query-builder/src/QueryBuilderRequest.php:55)

I fixed it by adding an extra null check:

https://github.com/Medalink/laravel-query-builder/blob/67ad6d786f34717a14b5d5fcc77735c86a18bfa9/src/QueryBuilderRequest.php#L54-L56

freekmurze commented 2 years ago

This PR addresses the issue by adding #[\ReturnTypeWillChange] to the offset methods.

Thanks! Could you fix this by adding : void instead of that attribute?

Medalink commented 2 years ago

@freekmurze using bool/void for the 4 methods work just as well. I was not sure how that would work with backwards compatibility but I updated the PR with return types that fix this without the need for #[\ReturnTypeWillChange]

freekmurze commented 2 years ago

Thanks!

ankurk91 commented 2 years ago

@freekmurze

mailcoach is using v3 of this package and we are getting same error while running on 8.1 :(

can you guys patch v3.0 as well?

freekmurze commented 2 years ago

We're not actively working on 3.0 anymore. If you need a fix for this, feel free to PR the desired changes and we'll consider merging it in.