NullabilityColumnPropertyTypeResolver - default column nullable false #191
Conversation
|
Thanks 👍 Tests fixtures have to be updated too. You can do it easily by: UT=1 vendor/bin/phpunit |
|
For note, this may be related with old issues duplicated for this: as noted at rectorphp/rector#7709 (comment) |
| @@ -25,7 +25,7 @@ class SimpleColumn | |||
| /** | |||
| * @ORM\Column(type="string") | |||
| */ | |||
| private ?string $name = null; | |||
| private string $name; | |||
There was a problem hiding this comment.
per noted at rectorphp/rector#7709 (comment) , the rule should be configurable instead, as except `#[ORM\Id], non-initiable property will got unitialized error quickly
There was a problem hiding this comment.
IMHO nullable:false Is default, so it should be also respected by property typehint. Otherwise it Is inconsistency between ORM definition and typehint.
IMHO this problem is only for ORM\Id. Wouldn't it be better to exclude for ORM\Id? And for others properties to respect nullable:false?
There was a problem hiding this comment.
Entity ID is magic, that's why allowed to be typed even never assigned inside class, other properties are not, the entity usage can be abused and cause random initialized error.
As far as the rule is configurable to allow enable fully typed based on docblock (disable by default), it is ok.
But make a typed property based on doc is incorrect generally, we seen a lot of legacy app that rely on docblock and never initialized, changing to typed broke everything.
|
@TomasVotruba I tested on local before, and the tests passed. Weird :-) Updated yet. UT=1 is briliant, I didn't know easy-testing. |
There was a problem hiding this comment.
The rule need to be configurable #191 (review)
to avoid unitialized error
jirinapravnik
force-pushed
the
nullable-column-default
branch
2 times, most recently
from
fb0973a
to
24aac61
Compare
Revert "update tests" commit 49c6af9.
jirinapravnik
force-pushed
the
nullable-column-default
branch
from
24aac61
to
899a32e
Compare
|
@samsonasik ok, I did it configurable with default true. It worked on old fixtures. I have problem with one phpstan rule and don't know how to get over it. |
|
|
||
| /** | ||
| * @see \Rector\Doctrine\Tests\CodeQuality\Rector\Property\TypedPropertyFromColumnTypeRector\TypedPropertyFromColumnTypeRectorTest | ||
| */ | ||
| final class TypedPropertyFromColumnTypeRector extends AbstractRector implements MinPhpVersionInterface | ||
| final class TypedPropertyFromColumnTypeRector extends AbstractRector implements MinPhpVersionInterface, AllowEmptyConfigurableRectorInterface |
There was a problem hiding this comment.
AllowEmptyConfigurableRectorInterface is removed at PR:
please use ConfigurableRectorInterface with default empty value instead.
|
I'm facing the same issue. I don't think the proposed configuration option really solves anything with regards to id columns. In my case I would love that Rector can add all the typehints automatically, but for the id columns I would need these to be nullable. So that would mean I either need to set the new option to So IMO there should be a second setting which is only applied to the Id columns (might be true/false/null and in case of null use the other option). |
| @@ -31,7 +31,7 @@ public function __construct( | |||
| ) { | |||
| } | |||
|
|
|||
| public function isNullable(Property $property): bool | |||
| public function isNullable(Property $property, bool $defaultNullableColumn): bool | |||
| { | |||
| $nullableExpr = $this->attributeFinder->findAttributeByClassArgByName( | |||
There was a problem hiding this comment.
I myself are not sure about this (existing) code. This as it threads #[Column(type: ...)], without nullable, as if the attribute isn't set at all. In which case it would test for the @Column annotation. While the existence of the attribute IMO needs to use either the configured nullable or, when absent, the default/fallback, and not looking for the @Column annotation.
|
@jirinapravnik are you planning on fixing the outstanding problems? otherwise I wouldn't mind opening a new PR to do it myself |
|
Hi @samsonasik , could you look into this one and finish so we have it main? I'd like to avoid staling the work that's done here. Thanks 🙏 |
|
@TomasVotruba I think this feature should not be added, except a user need to, and want to contribute to make this happen. Unitialized property can make a bug very quickly without make it nullable, except doctrine ID that mostly magic. |
|
@samsonasik I see, that's why it got stale. Let's close this PR then, thanks How about this one? #197 |
|
still no from me, |
Issue #190