Nullable flag on constructor property promotion, closes #183 #184
Conversation
Grundik
force-pushed
the
fix-183
branch
6 times, most recently
from
6da6607 to
0531bad
Compare
src/Generator/ParameterGenerator.php
Outdated
| /** | ||
| * @return ?TypeGenerator | ||
| */ | ||
| public function getTypeObject() | ||
| { | ||
| return $this->type; | ||
| } |
There was a problem hiding this comment.
Exposing the type to the outside world seems like a regression here, as it will reduce the ability to change the codebase later on.
| if ( | ||
| null !== $type | ||
| && ($typeObject = $generator->getTypeObject()) | ||
| && $typeObject->getNullable() | ||
| ) { | ||
| $type = '?' . $type; | ||
| } |
There was a problem hiding this comment.
Wondering if the $type of ParameterGenerator could be exposed internally only: we could then get rid of the string type overall
There was a problem hiding this comment.
Changed that to exposing only nullable flag.
There was a problem hiding this comment.
@Grundik I don't have a clear mind on the approach here yet, but I'm trying to think of a design that removes public, and ideally avoids protected (although the inheritance design is already out of the bag)
There was a problem hiding this comment.
The other way I see, is to make ParameterGenerator#generateTypeHint public, this would not require exposing getNullable().
But it looks like no big deal to expose that: there a lot of other flags exposed in a same way: passedByReference, vadiadic and such.
There was a problem hiding this comment.
Right, I'm much in "damage control" mode here, since the generator, builder and reflection-alike structure has been merged together in the past, making changes to internal structures lead to major versions :)
I'll sleep over it, but I'd rather not expose more than is strictly needed for ->generate() to work.
There was a problem hiding this comment.
Currenly there are no simple way to get this flag. E.g. parsing result of ParameterGenerator->generate() would be ugly, fragile, slow and overcomplicated.
There was a problem hiding this comment.
Nono, I'm absolutely not suggesting that we go down the parsing way :)
Signed-off-by: Grundik <[email protected]>
|
Just to be clear: this is not the fix for Ocramius/ProxyManager#784, there are at leas two different issues:
|
Description
See #183 for details.