-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve docs in relation to PhpStan failure #10
Conversation
What do you think, @jakubbartel? |
Great! Just for note: when I use php-component as a composer dependency I need to use only the first of |
example/MyConfigDefinition.php
Outdated
|
||
class MyConfigDefinition extends \Keboola\Component\Config\BaseConfigDefinition | ||
{ | ||
/** | ||
* @return ArrayNodeDefinition|NodeDefinition | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need ArrayNodeDefinition here? parameters is also always an object (or should be normalized to), so NodeDefinition
should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it... is should be always ArrayNodeDefinition
. Because the parameters are always array. If it were NodeDefinition
, it would fail because children()
is does not exist there :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i always mix the classes, but the point is that only one of them is sufficient
I also came across this - #10 (comment) It's easy to fix, no rocket science, but blind copy & paste of |
I changed it little bit more. As @odinuv noted, the issue was partly with intersection typehint (which was a reminiscence from earlier version). When I removed it the PhpStan errors are gone. So I replaced the note with a more general note. |
16af2ed
to
4f8bac0
Compare
Ještě zvažuju, jak to udělat s tagnutím nové verze. Tím, že se to srovnalo a přibyly typehinty, tak to není zpětně kompatibilní. Ale nevím, jestli kvuli tomu hned vydávat novou major verzi. Zatím to používáme stejně jen my a @jakubbartel ne? Ale formálně správně by bylo asi 2.0.0. Co myslíte? |
Klidně novou major verzi. |
OK, done. |
Fixes #8