Drop PHP 7, Support PHP 8.2 #90
Conversation
Signed-off-by: Roger Vilà <[email protected]>
|
Depends on laminas-api-tools/api-tools#96 |
Bumps to most recent releases of all api-tools components. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
All of these are due to array shapes. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Several tests have ".dist" config files that a setUp process copies to a path omitting the suffix. That file is then _included_ by the test in order to do assertions. Psalm was flagging the include as invalid due to the file not existing in a clean setup (which it will not). Signed-off-by: Matthew Weier O'Phinney <[email protected]>
| * @param array<string, mixed> $config | ||
| */ | ||
| public function __construct($name, $config) | ||
| public function __construct(public string $name, public array $config) |
There was a problem hiding this comment.
I see a number of these changes in the various entity classes (adding constructor property promotion with explicit types).
Overall, I think they are fine; these were the documented types in annotations, and what we test against; further, using constructor property promotion ensures the properties are actually declared, which they were not before.
However, it is a subtle BC break for anybody extending these classes. @Ocramius , what are your thoughts? Should we go ahead with these for next minor, push to next major, or ask to revert them?
There was a problem hiding this comment.
On the constructor, they are mostly fine due to them not affecting LSP checks
There was a problem hiding this comment.
since the property is dynamic, it is bc break https://3v4l.org/vLe8v
Rector can cover that:
- dynamic: skipped https://getrector.com/demo/9cbff805-62d4-4557-baab-ea1875b11127
- defined property: applied
a. typed https://getrector.com/demo/1ef76619-2b7f-43d4-9c7b-0cd009d69b45
b. not typed https://getrector.com/demo/085e9f92-ed93-45b4-ace9-1673c8770816
Instead, I suggest add public property instead:
public $name;
public $config;There was a problem hiding this comment.
anyway, Rector has CompleteDynamicPropertiesRector to auto add public property if needed:
https://getrector.com/demo/a8f2c237-a12c-4fae-9f92-eba6677a53d4
There was a problem hiding this comment.
So, if you combine CompleteDynamicPropertiesRector + ClassPropertyAssignToConstructorPromotionRector , you will get it processed but kept the @param without adding new type
https://getrector.com/demo/26c0e45b-a208-4a5f-acd8-f82e11bbda51
There was a problem hiding this comment.
On summary, just remove the type, and it will be safer:
| public function __construct(public string $name, public array $config) | |
| public function __construct(public $name, public $config) |
There was a problem hiding this comment.
since the property is dynamic, it is bc break
Right, but these are (a) essentially internal, and (b) the expectation is that the properties are present. I'd argue it actually tightens the contract and makes it more reliable.
There was a problem hiding this comment.
@weierophinney I see, it need @internal flag then
There was a problem hiding this comment.
remove the type,
We should have been validating the type all along, however. This ensures the type is correct.
Signed-off-by: Roger Vilà [email protected]
Description
Drop PHP 7, Support PHP 8.2