FEATURE: Increase PHP 8 compatibility #2381
Conversation
# Conflicts: # Neos.Kickstarter/composer.json
| @unlink($this->lockFlagPathAndFilename); | ||
| if (file_exists($this->lockFlagPathAndFilename)) { | ||
| @unlink($this->lockFlagPathAndFilename); | ||
| } |
There was a problem hiding this comment.
The shutup operator seems to be ignored for PHP 8 - at least I got a Warning that the file didn't exist
There was a problem hiding this comment.
Yep, see https://github.com/neos/flow-development-collection/pull/2287/checks?check_run_id=1974309752#step:12:28
PHP 8 will behave differently and I guess we need to try/catch here (our errorHandler will convert the error to an Exception) or introduce the "double shrug 🤷♀️🤷♂️()" function.
https://php.watch/versions/8.0/fatal-error-suppression
| return null; | ||
| } | ||
| return $type instanceof \ReflectionNamedType ? $type->getName() : (string)$type; | ||
| return $type->isBuiltin() ? $type->getName() : null; |
There was a problem hiding this comment.
According to https://www.php.net/manual/en/migration80.incompatible.php#migration80.incompatible.reflection
ReflectionType::isBuiltin() method has been moved to ReflectionNamedType. ReflectionUnionType does not have it.
Not sure if the change addresses this correctly!
There was a problem hiding this comment.
I think we still need to do the instanceof check, because else we lose compatibility with PHP 7.x. Also, we did not invoke isBuiltin() here at all?
There was a problem hiding this comment.
the instanceof check is in line 60 now but I'm not sure if 61 should be return (string)$type instead
There was a problem hiding this comment.
In PHP8 there are only ReflectionNamedType and ReflectionUnionType. Hence the question becomes: How should we handle a union of a builtin type with other types here?
For PHP >= 7.1 only ReflectionNamedType exists, so returning null should be okay. If we want to be super sure, it should be PHP < 8 ? (string)$type : null
| @@ -149,7 +149,7 @@ public function dispatch(string $signalClassName, string $signalName, array $sig | |||
| } | |||
|
|
|||
| foreach ($this->slots[$signalClassName][$signalName] as $slotInformation) { | |||
| $finalSignalArguments = $signalArguments; | |||
| $finalSignalArguments = array_values($signalArguments); | |||
There was a problem hiding this comment.
Not sure if that's the proper fix, but I got an error
named argument "$response" doesn't exist
(or similar) because PHP 8 tries to map this to a named argument if the array contains associative keys
There was a problem hiding this comment.
FYI: WIth PHP 8.1 this won't be required any longer (https://wiki.php.net/rfc/array_unpacking_string_keys)
There was a problem hiding this comment.
Yeah, but will be some time until we have PHP >= 8.1 minimum :)
| if ($type === T_STRING) { | ||
| $namespaceParts[] = $value; | ||
| if ($type === T_STRING || $type === T_NAME_FULLY_QUALIFIED || $type === T_NAME_QUALIFIED) { | ||
| $namespaceParts[] = ltrim($value, '\\'); |
There was a problem hiding this comment.
apparently the behavior of token_get_all() has changed..
|
See #2287 - you could add your changes there and would immediately get feedback from the running PHP 8 builds |
|
FYI: I merged these changes into #2287 prefering yours - so we can close this and continue on the other PR with getting us PHP8 ready |
Related: #2233