[Config] Define TreeBuilder default generic type
#62616
Conversation
carsonbot
changed the title
| /** | ||
| * Generates the configuration tree builder. | ||
| * | ||
| * @return TreeBuilder<'array'> |
There was a problem hiding this comment.
@nicolas-grekas unsure about this, is there any case where the getConfigTreeBuilder returns something else as an array?
There was a problem hiding this comment.
the interface allows it 🤷
|
Fabbot error looks unexpected and not related to the changes. |
| /** | ||
| * @return TreeBuilder<'array'> | ||
| */ |
There was a problem hiding this comment.
Not sure why psalm wants this be added shouldn't it not just use interface return type here and not force redefinition?
| * This class provides a fluent interface for defining an array node. | ||
| * | ||
| * @template TParent of NodeParentInterface|null | ||
| * @template TParent of NodeParentInterface|null = null |
There was a problem hiding this comment.
Not sure about the default here?
|
Stuck with psalm and the correct defaults if somebody has any hints to tackle this correctly to make it as easy as possible to create config builders without devs need to think about any generics. |
| * This is the entry class for building a config tree. | ||
| * | ||
| * @template T of 'array'|'variable'|'scalar'|'string'|'boolean'|'integer'|'float'|'enum' | ||
| * @template T of 'array'|'variable'|'scalar'|'string'|'boolean'|'integer'|'float'|'enum' = 'array' |
There was a problem hiding this comment.
this shouldn't be needed, the $type = 'array' default is enough, phpstan understands it.
If other tools don't, that should be reported to them
There was a problem hiding this comment.
enough for usage of new TreeBuilder maybe but not for param type with function method(TreeBuilder $treeBuilder) or return type method(): TreeBuilder to defaults there to 'array'.
There was a problem hiding this comment.
I ran into the exact problem today and can confirm that PHPStan is indeed reporting an issue:
Method MyBundle\DependencyInjection\Configuration::getConfigTreeBuilder() return type with generic class Symfony\Component\Config\Definition\Builder\TreeBuilder does not specify its types: T
carsonbot
changed the title
|
I'm kind of stucked here as not correctly understanding here the generics in all cases. But we sure should find a solution this kind of PHPStan errors not appear for everybody try to create a bundle config. If somebody want to take this over I'm fine. |
OskarStark
changed the title
TreeBuilder default generic type
nicolas-grekas
left a comment
nicolas-grekas
left a comment
There was a problem hiding this comment.
Let's merge this and iterate as needed.
psalm reports are about the internals, but once merged, it won't propagate to userland AFAIK.
nicolas-grekas
force-pushed
the
patch-10
branch
from
61adfa2 to
d21ad61
Compare
|
Thank you @alexander-schranz. |
… generic (alexander-schranz) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Config] Add default generic to Configuration to TParent generic | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no <!-- if yes, also update src/**/CHANGELOG.md --> | Deprecations? |no <!-- if yes, also update UPGRADE-*.md and src/**/CHANGELOG.md --> | Issues | Fix #... <!-- prefix each issue number with "Fix #"; no need to create an issue if none exists, explain below --> | License | MIT Missed in #62616. Define all TParent which are nullable as default null. Commits ------- c6ff984 [Config] Add default generic to Configuration to TParent generic
5 participants
The generics make sense internal but are very confusing for all people not knowing about internal behaviours and hard to guess whats corrct so we should provide defaults for them. Similar to #61805 add some defaults.
Stumbled over this during upgrade StofDoctrineExtensionBundle: