Conversation
mnapoli
left a comment
mnapoli
left a comment
There was a problem hiding this comment.
Thanks! I have written a few comments inline, also could you add a changelog entry in the 6.0 section?
tests/UnitTest/ContainerGetTest.php
Outdated
| $this->assertEquals('foo', $container->debugEntry('string')); | ||
| $this->assertEquals('string', $container->debugEntry('entry')); | ||
| $this->assertEquals('stdClass', $container->debugEntry('object')); | ||
| } |
There was a problem hiding this comment.
This test is ContainerGetTest.php and it's supposed to test the get() method.
I think it would make more sense to move those 2 new tests into a new test class in IntegrationTest (instead of UnitTest) that would test the debug features, i.e. DebugTest ?
| * Get defined container entries. | ||
| * | ||
| * @return array | ||
| */ |
There was a problem hiding this comment.
You can add the array return type here and remove the same line that is in the phpdoc (PHP 7 FTW ^^)
There was a problem hiding this comment.
I think code base must be reviewed on missing type hinting hunt, for example on Container::has
There was a problem hiding this comment.
I cannot add it for has() and get() because those are defined in PSR-11 unfortunately.
| * @throws InvalidDefinition | ||
| * | ||
| * @return string | ||
| */ |
There was a problem hiding this comment.
Could you add the strict types here too? (parameter + return type)
|
|
||
| return is_object($entry) ? get_class($entry) : gettype($entry); | ||
| } | ||
|
|
There was a problem hiding this comment.
You could throw a NotFound exception? (it wouldn't be a huge difference but since the exception exists let's use it)
|
Thanks! |
2 participants
covers container entries debug dumper as per comment on #152