Check is nullable doctrine annotation #324
Conversation
...erty/TypedPropertyFromToOneRelationTypeRector/Fixture/doctrine_attribute_many_to_one.php.inc
Outdated
Show resolved
Hide resolved
| @@ -39,7 +39,7 @@ class DoctrineManyToOne | |||
| * @ORM\ManyToOne(targetEntity="Rector\Doctrine\Tests\CodeQuality\Rector\Property\TypedPropertyFromToOneRelationTypeRector\Source\Company", inversedBy="userBuildings") | |||
| * @ORM\JoinColumn(name="company_id", referencedColumnName="id", nullable=false) | |||
| */ | |||
| private ?\Rector\Doctrine\Tests\CodeQuality\Rector\Property\TypedPropertyFromToOneRelationTypeRector\Source\Company $company3 = null; | |||
| private \Rector\Doctrine\Tests\CodeQuality\Rector\Property\TypedPropertyFromToOneRelationTypeRector\Source\Company $company3; | |||
There was a problem hiding this comment.
I think this would crash, as this property is not defined via constructor. Often Symfony/forms use magic setters to set value here and they would crash as well:
There was a problem hiding this comment.
yes, that's true. But isn't this than the issue of the framework or the usage?
So yeah, if the field is nullable: false i would say the property itself should also be not null. Doctrine will load the property correctly. if someone will use a partial load of a entity this will also break.
But what is the alternative? Mark the properties as nullable and look for getter and setter and mark them as not nullable?
or what do you think about an configurable option for that?
There was a problem hiding this comment.
The code has to be valid after migration, and we have to take context of use into an account.
Here e.g. the property would have to be set at least in the constructor to make it typed.
In case constructor is present and property is set, the ? can be removed. I think best way would be to make this optionally configurable as you suggest 👍
There was a problem hiding this comment.
first time i used this configured rules :D
Hopefully i implemented them correct. Sadly i didn't find a good way to show it in the automated docs which option is available, which one is the default behavior and how the configuration will change the output.
I only added two examples for the Rule and add a new fixture folder for the configured tests
marcelthole
force-pushed
the
check-is-nullable-doctrine-annotation
branch
from
e4ed6cc
to
59142f6
Compare
marcelthole
force-pushed
the
check-is-nullable-doctrine-annotation
branch
from
59142f6
to
7c3e131
Compare
| { | ||
| public const FORCE_NULLABLE = 'force_nullable'; | ||
|
|
||
| private bool $forceNullable = true; |
There was a problem hiding this comment.
Default value should be false, not to change current behavior and create BC break.
|
I love it, very good work 👍 |
|
This is the current default. If you take a look into the tests, you should see that the rule without configuration will use the nullable types again and only the configured tests with forceNullable=false are using the new logic.
Or did I miss something here? A BC break was not expected here.
Am 1. Juli 2024, 21:12, um 21:12, Tomas Votruba ***@***.***> schrieb:
…
@TomasVotruba commented on this pull request.
> {
+ public const FORCE_NULLABLE = 'force_nullable';
+
+ private bool $forceNullable = true;
Default value should be `false`, not to change current behavior and
create BC break.
--
Reply to this email directly or view it on GitHub:
#324 (review)
You are receiving this because you authored the thread.
Message ID:
***@***.***>
|
|
Ah, you're right. I assumed "force" is the more strict case :) All good, thank you 👏 |
fix #305