search

laravel-json-api / eloquent

Serialize Eloquent models to JSON API resources
MIT License
12 stars 15 forks source link

Support for PHP 8.1 #21

Closed danny-allen closed 2 years ago

danny-allen commented 2 years ago

I've been working on a new application and we decided to start with PHP version 8.1 since we're months away from any sort of app-launch and PHP 8.1 is due for GA release on 25th Nov 2021 (3 days).

I tried using the Laravel JSON API package and the only thing that seems like an issue for compatibility with PHP 8.1 is the use of the name ReadOnly. readonly is a reserved keyword in PHP 8.1 and causes a problem with the Interface and Trait in this package.

Thanks for all your work on these packages!

I've submitted a PR that appears to fix the issue. Hopefully it's of help:

https://github.com/laravel-json-api/eloquent/pull/20

lindyhopchris commented 2 years ago

Thanks for reporting. Does this mean you get a fatal error in 8.1 for those classes/traits?

Thanks also for the PR... I'm not entirely sure I like OnlyRead as a name though. My preference would be IsReadOnly, which presumably would be allowed? If you wanted to adjust the PR that'd be great, but I'd understand if you felt that was a waste of your time and I'd do it instead!

Bit of a pain I'll need to do a major release just for this change. I probably won't release immediately, because if I'm doing a major release it'd be ideal for me to see what other breaking tickets I can deal with in one release.

danny-allen commented 2 years ago

Not a problem!

I get the following:

ParseError syntax error, unexpected token "readonly" at vendor/laravel-json-api/eloquent/src/Contracts/Fillable.php:24

I agree with the OnlyRead I couldn't think of anything better at the time, but IsReadOnly is definitely better. I'll look to change this very shortly.

Bit of a pain I'll need to do a major release just for this change. I probably won't release immediately, because if I'm doing a major release it'd be ideal for me to see what other breaking tickets I can deal with in one release.

No worries - I agree, I think they should have added this in v8.0 or waited for v9.

lindyhopchris commented 2 years ago

Great, thanks for your responses! Yeah agree it's odd that this is coming in on 8.1.

danny-allen commented 2 years ago

No worries. I've made those changes and ran our unit tests - all is working well. PR is updated.

For the time being we'll develop with our forked version of the repository - until this is merged in and released. Obviously if we find any other PHP 8.1 issues we'll try to resolve and keep you posted.

Thanks!

lindyhopchris commented 2 years ago

Tagged as 2.0.0 - sorry for delay, was waiting for Laravel 9 to be tagged.