INJECTOR: Add type declarations to properties #45
Conversation
src/Injector.php
Outdated
|
|
||
| /** @var ConfigInterface */ | ||
| protected $config; | ||
| protected DefinitionInterface $definition; |
There was a problem hiding this comment.
This type addition is a BC break for inheritance scenarios 🤔
There was a problem hiding this comment.
Would you like me to add back the docblock or is there a better way to account for the BC break?
There was a problem hiding this comment.
On a side note, is there a way for me to detect BC breaks like those up front?
There was a problem hiding this comment.
I wrote https://github.com/Roave/BackwardCompatibilityCheck, which is somewhat able to detect some of these :-)
I'd really like to integrate it with our CI: laminas/laminas-ci-matrix-action#60
And also missing method return types Signed-off-by: Remy Bos <[email protected]>
sjokkateer
force-pushed
the
improvement/injector
branch
from
039efd5 to
2b357c0
Compare
|
|
||
| /** @var string[] */ | ||
| protected $instantiationStack = []; | ||
| protected array $instantiationStack = []; |
There was a problem hiding this comment.
this is BC break, only private property that allowed the type to be changed when class is not final
There was a problem hiding this comment.
I see, thanks for the insight. Does that mean the other protected properties I modified should be reverted back to using docblocks as well?
| * @throws ClassNotFoundException | ||
| * @throws RuntimeException | ||
| */ | ||
| public function create(string $name, array $parameters = []) | ||
| public function create(string $name, array $parameters = []): ?object |
There was a problem hiding this comment.
this is also BC break when extended class override it, ref https://3v4l.org/QTiTt
| * @throws InvalidCallbackException | ||
| * @throws ClassNotFoundException | ||
| */ | ||
| protected function createInstance(string $name, array $params) | ||
| protected function createInstance(string $name, array $params): object |
There was a problem hiding this comment.
this is also BC break when extended class override it, ref https://3v4l.org/OgOHS
There was a problem hiding this comment.
I see now, thanks again for the insight.
Sorry for my lack of knowledge and wasting your and @Ocramius time but I think this pull request can be closed/ignored then.
And also missing method return types
Signed-off-by: Remy Bos [email protected]
Description
This PR adds type declarations to the
Injector::classproperties as well as update some of the missing method return types.