Skip to content
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

Add support for inherited nullability from PHP #11814

Open
wants to merge 2 commits into
base: 3.4.x
Choose a base branch
from

Conversation

Rixafy
Copy link

@Rixafy Rixafy commented Jan 29, 2025

Closes #9744 and #11797, relates to #11620.

It's made as opt-in feature so we don't cause BC break, it can be enabled with $config->setInferPhpNullability(true).

Example code before:

#[Column(nullable: true)]
private ?string $property = null;

#[ManyToOne]
#[JoinColumn(nullable: false)]
private Entity $entity;

#[OneToOne] // db = nullable, php = not nullable
private Entity $test;

Example code after:

#[Column] 
private ?string $property = null;

#[ManyToOne] 
private Entity $entity;

#[OneToOne]
#[JoinColumn(nullable: true)] // to keep previous behavior
private Entity $test;

I have already tested it also on mid-sized project and new generated migration was ok (I already migrated it, ~25 queries), with default configuration. It helped me discover few database rows with null where should not be null, and it would cause app error if those entities were loaded.

I'm not sure how to treat properties without type, maybe we should make them nullable as well when this feature is enabled, since null is valid value for them. For now I kept original behavior for untyped properties but I think it would be better to make them nullable by default (JoinColumn already is, just Column is not) when this feature is enabled.

@Rixafy Rixafy force-pushed the nullability branch 4 times, most recently from 61b1134 to 80d2ee7 Compare January 30, 2025 01:16
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've limited inferring nullability to attribute mapping while type inference works with any driver. Why's that?

src/Mapping/Column.php Outdated Show resolved Hide resolved
src/Mapping/Column.php Outdated Show resolved Hide resolved
@Rixafy
Copy link
Author

Rixafy commented Jan 30, 2025

You've limited inferring nullability to attribute mapping while type inference works with any driver. Why's that?

It seemed like easier option to start with, and I didn't know codebase very well yesterday, now I see that I can do it maybe in ClassMetadata.

@Rixafy Rixafy force-pushed the nullability branch 2 times, most recently from af3f9cb to b92befd Compare January 30, 2025 16:24
@Rixafy
Copy link
Author

Rixafy commented Jan 30, 2025

I did a rewrite and now it's working for all drivers via Configuration::setInferPhpNullability, because it's saved only on ClassMetadata level, and attribute driver can access it and if it's enabled, it will set null to nullable array key, which fails isset in ClassMetadata.

Btw in next major version, this could be the default behavior.

Now I noticed I selected wrong target branch, because I noticed one bug with default referenced column name and found out it was fixed and there is already 3.4.x branch.. I'll look at it in a few hours and I will switch it.

@Rixafy Rixafy requested a review from derrabus January 30, 2025 16:26
@Rixafy Rixafy force-pushed the nullability branch 6 times, most recently from 23c1c5d to 1795e61 Compare January 31, 2025 15:51
@Rixafy Rixafy changed the base branch from 3.3.x to 3.4.x January 31, 2025 15:51
@Rixafy
Copy link
Author

Rixafy commented Jan 31, 2025

I rebased it onto 3.4.x, it's ready for review, one thing I'm not sure about is to how to treat variables without type, I think it will be better to have them nullable (JoinColumn already is, but Column isn't) when using inferPhpNullability setting.

For now, I left it how it was, but it's more consistent to have them both nullable when using this setting, I don't use untyped properties so I don't mind, but it's a thing to consider and it will be difficult to change it in the future without BC break.

@derrabus
Copy link
Member

one thing I'm not sure about is to how to treat variables without type,

If there's no type, there's nothing we could infer nullability from. The old defaults are good for those cases.

@Rixafy
Copy link
Author

Rixafy commented Jan 31, 2025

one thing I'm not sure about is to how to treat variables without type,

If there's no type, there's nothing we could infer nullability from. The old defaults are good for those cases.

That's also my reasoning why I didn't added it in a first place, but then I started doubting it, since it's named as inferPhpNullability (or maybe inferPropertyNullability would be better, but longer) not type nullability (to keep those door open), so it's ok with this naming, because by PHP definition, property can be null.

I like the error-prevention of that nullability, because if DB schema is up to date with code, and no nullable is defined in attributes, there is zero chance of null related issues when loading/accessing entity data or persisting entity into the DB.

@Rixafy Rixafy force-pushed the nullability branch 3 times, most recently from 7e76fe3 to 307736e Compare February 1, 2025 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants