feat: Support PHP 8.5 #1689
feat: Support PHP 8.5 #1689
Conversation
| public function guessTargetContextClass(ContextEnvironment $environment): ?string | ||
| { | ||
| if ($environment->hasContextClass($this->contextClass)) { | ||
| if (is_string($this->contextClass) && $environment->hasContextClass($this->contextClass)) { |
There was a problem hiding this comment.
See the commit message for details of this fix.
There was a problem hiding this comment.
I would probably write it as $this->contextClass !== null instead.
There was a problem hiding this comment.
I would have too, historically.
However if String was a class rather than a scalar in PHP, then Rector's FlipTypeControlToExclusiveTypeRector (which is part of the code quality set that we have enabled) would flip that to $this->contextClass instanceof String.
Obviously it's not a class, and therefore Rector has no opinion on this condition - but it felt to me like is_string(..) was more in the style of how (object-property) conditions are expressed in the rest of our codebase.
The property is strict typed as ?string so there's no functional difference either way. Happy to change it if you want?
There was a problem hiding this comment.
That rector rule is quite opinionated though (and I would probably never use it in my own projects)
There was a problem hiding this comment.
@carlos-granados do you have an opinion? I have no particularly strong feelings either way but conscious you've been most involved in picking Rector rules to apply.
There was a problem hiding this comment.
No strong opinion one way or the other, let's just keep it as it is
Adds PHP 8.5 to our build matrix and supported versions.
`FixedContextIdentifier` is always created and registered by the `ContextSnippetsController` with an injected `contextClass` based on the `--snippets-for` CLI option - see https://github.com/Behat/Behat/blob/0d4162bbf8401bfba8085a25f427eabe6d00cead/src/Behat/Behat/Context/Cli/ContextSnippetsController.php#L65. If this option has not been specified on the CLI then the `contextClass` property will be `null`. This was causing us to call `ContextEnvironment::hasContextClass` with a `null` despite that method being typed as taking only `class-string<Context>` (the method currently only has a phpdoc typehint). This means `InitializedContextEnvironment::hasContextClass()` then attempts to use `null` as an array key. Up to PHP 8.4 this worked as expected, but from 8.5.0 it triggers a deprecation. Therefore, we now explicitly check if the value is null in `FixedContextIdentifier` to ensure that we only call the `->hasContextClass()` with a valid string value.
| # MacOS on latest PHP only | ||
| - php: 8.4 |
There was a problem hiding this comment.
I have not yet bumped the MacOs or Windows jobs to 8.5 - I think we should wait until there's a stable 8.5 release for that to avoid any packaging issues / RC changes affecting the only builds we run on these platforms.
Adds PHP 8.5 to our build matrix and supported versions.