Fixes php 8.4 deprecation warnings like: "Implicitly marking paramete… #607
Conversation
…r ... as nullable is deprecated, the explicit nullable type must be used instead in ..." Example Warning: PHP Deprecated: Kalnoy\Nestedset\NodeTrait::create(): Implicitly marking parameter $parent as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/vendor/kalnoy/nestedset/src/NodeTrait.php on line 754 And fixes "Class "Laravel\SerializableClosure\Support\ReflectionClosure" not found" when running the tests.
|
@lazychaser Can this be merged? |
|
@lazychaser can you take a look? |
|
@lazychaser can you merge this? |
|
@lazychaser can you take a look please? |
|
If you could have a look it would be nice. The changelog isn’t that big as I see it. |
|
@julesgraus you might want to rebase the branch because the L12 compatibility was merged. Maybe then @jonnott would have a second to look at it. |
|
Done |
Fix quote problem due to incorrect rebase / merge
| "illuminate/support": "^7.0|^8.0|^9.0|^10.0|^11.0|^12.0", | ||
| "illuminate/database": "^7.0|^8.0|^9.0|^10.0|^11.0|^12.0", | ||
| "illuminate/events": "^7.0|^8.0|^9.0|^10.0|^11.0|^12.0" | ||
| "illuminate/events": "^7.0|^8.0|^9.0|^10.0|^11.0|^12.0", | ||
| "laravel/serializable-closure": "^2.0" |
There was a problem hiding this comment.
@julesgraus What in this change-set causes the need for laravel/serializable-closure ?
There was a problem hiding this comment.
At this moment I cannot tell you that because it is months ago that I’ve made this pull request. It’s something in the existing tests from this package that needs it (as stated in the description of this pull request). Without it you will get an error. At the moment I don’t have much spare time to have a deeper look into it
There was a problem hiding this comment.
It fixes the error: Class "Laravel\SerializableClosure\Support\ReflectionClosure" not found when running the tests
There was a problem hiding this comment.
@julesgraus Tests all failing in CI - see checks output.
There was a problem hiding this comment.
The php 8.1 and 8.2 versions should be fixed now. The phpunit.xml did state that every .php file in ./tests/ is a test file. But that's not true because the files in tests/data and tests/models aren't tests. That is why those tests were failing. So I've excluded those directories in the phpunit.xml file.
Then we have the php versions 8.0 and older. Every php version from php 8.0 and before is end of life: https://www.php.net/supported-versions.php. As the supported versions page from php states:
Users of this release should upgrade as soon as possible, as they may be exposed to unpatched security vulnerabilities.
I used homebrew to install [email protected]. That worked. But when I composer install the project by using the php version directly like this
/opt/homebrew/Cellar/[email protected]/8.0.29_1/bin/php /usr/local/bin/composer install
I do get the error
dyld[21759]: Library not loaded: /opt/homebrew/opt/icu4c/lib/libicuio.73.dylib
Referenced from: <869801BD-D63F-3A1A-B675-399400CD6FB7> /opt/homebrew/Cellar/[email protected]/8.0.29_1/bin/php
Reason: tried: '/opt/homebrew/opt/icu4c/lib/libicuio.73.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/opt/icu4c/lib/libicuio.73.dylib' (no such file), '/opt/homebrew/opt/icu4c/lib/libicuio.73.dylib' (no such file), '/usr/local/lib/libicuio.73.dylib' (no such file), '/usr/lib/libicuio.73.dylib' (no such file, not in dyld cache), '/opt/homebrew/Cellar/icu4c@77/77.1/lib/libicuio.73.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/icu4c@77/77.1/lib/libicuio.73.dylib' (no such file), '/opt/homebrew/Cellar/icu4c@77/77.1/lib/libicuio.73.dylib' (no such file), '/usr/local/lib/libicuio.73.dylib' (no such file), '/usr/lib/libicuio.73.dylib' (no such file, not in dyld cache)
[1] 21759 abort /opt/homebrew/Cellar/[email protected]/8.0.29_1/bin/php /usr/local/bin/composer install
Now, when I run this command brew install icu4c@73 to install icu4c at version 73, I do get this error:
No available formula with the name "icu4c@73". Did you mean icu4c@77, icu4c@76, icu4c@75 or icu4c@74?
It seems that icu4c@73 isn't available anymore.
There was a problem hiding this comment.
@julesgraus @lazychaser Do you think we should drop support for all EOL'd PHP versions?
There was a problem hiding this comment.
Personally I think we should. Just because of the recommendations of php and the security concerns.
To be hones, I could work on the other php versions to by using docker, but that feels like a waste of time because those php versions are EOL.
Anyways, thanks for your help with this PR. Good job.
There was a problem hiding this comment.
@julesgraus Whilst I've just stepped in to lighten @lazychaser's load in maintaining this repo, so I'm not any authority .. does it feel like such a change (dropping EOL PHP versions) should be a major version jump (i.e. 7.x), or could it just be 6.1 ?
There was a problem hiding this comment.
I understand. I do think that it's a major change. Let me explain why: Users with version ^6 might run this package in a php version we do like to drop support for. When they run the composer update command and they have the version of this package set to ^6, it means it would update to any new version of this package as long as it is less than version 7. This causes their app to break.
Maybe we should create a v7 branch? If so, I'll cherry pick the commit on it
…em as tests files because of the suffix defined in the phpunit config.
Fixes php 8.4 deprecation warnings like: "Implicitly marking parameter ... as nullable is deprecated, the explicit nullable type must be used instead in ..."
Example Warning: PHP Deprecated: Kalnoy\Nestedset\NodeTrait::create(): Implicitly marking parameter $parent as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/vendor/kalnoy/nestedset/src/NodeTrait.php on line 754
And fixes "Class "Laravel\SerializableClosure\Support\ReflectionClosure" not found" when running the tests.
I've discovered more issues with php docblocks that can be fixed in different ways, but i wanted to keep this pull request as small as possible.