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

PHP 8.1 Support #702

Merged
merged 3 commits into from
Dec 26, 2021
Merged

PHP 8.1 Support #702

merged 3 commits into from
Dec 26, 2021

Conversation

Medalink
Copy link
Contributor

@Medalink Medalink commented Dec 24, 2021

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
Copy link
Member

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
Copy link
Contributor Author

Medalink commented Dec 25, 2021

@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 freekmurze merged commit 6466956 into spatie:main Dec 26, 2021
@freekmurze
Copy link
Member

Thanks!

@Medalink Medalink deleted the feature/php81 branch December 26, 2021 20:54
@ankurk91
Copy link

ankurk91 commented Jan 7, 2022

@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
Copy link
Member

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.

{
return isset($this->subject[$offset]);
}

public function offsetGet($offset)
public function offsetGet($offset): bool
Copy link

@ankurk91 ankurk91 Jan 9, 2022

Choose a reason for hiding this comment

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

offsetGet should return mixed (Can return all value types) type

https://www.php.net/manual/en/arrayaccess.offsetget.php

https://php.watch/versions/8.0/mixed-type

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

3 participants