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.

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.

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.

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.

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.

I would like to prepare PR for phpstan/phpstan-doctrine, so it can read the config ang figure out how to check nullable types, but I don't know if this naming is ok, and if it gets into the 3.4 release.

@derrabus do you think I should change it from inferPhpNullability to inferPropertyNullability?

Hello, I would really appreciate it if someone could reply. I mentioned multiple people who know what’s going on, and I even asked about it on Slack two months ago, but I got no reply.

Because of that small BC break #12108 where $nullable became bool|null, if I revert it back to bool and add $nullableSet to indicate whether the value was passed intentionally, I would introduce even worse BC break, because if anyone is passing "null" into it, it will break.

So now I think I should keep JoinColumn attributes as they are (where $nullable is now bool|null).

But what about Doctrine\ORM\Mapping\Column? I'm handling it by adding $nullableSet property to indicate whether $nullable was set (see https://github.com/doctrine/orm/blob/7649263a91cc206d926c18ced045c832190c540c/src/Mapping/Column.php).

This way public property $nullable stays bool, should I make it same as JoinColumn where it became bool|null and cause small BC break (for consistency), or should I keep my current solution?

If no one answers by the end of the week, I'm keeping Column attribute with $nullableSet and I'll rebase my JoinColumn to current version introduced by PR I mentioned.

So I rebased it as I said and tested it in my project (works the same), failed pipeline seems unrelated (it's failed also in 3.6.x).

Could I please get a review and could you add this to 3.6 milestone? @greg0ire @beberlei @derrabus @SenseException

I remember a lengthy discussion about this feature at the 2023 hackathon, although I fail to recall the details.

One thing I remember was that people may have a need for nullable properties to be able to bind form systems (like Symfony Forms) to entities, where fields may need to be null in code but not in DB… does that make sense?

One thing I remember was that people may have a need for nullable properties to be able to bind form systems (like Symfony Forms) to entities, where fields may need to be null in code but not in DB… does that make sense?

I'm not familiar with symfony/forms (I'm using nette/forms and manual mapping), but inherited nullability is not forced, you can always override it with nullable attribute, just default value will be same as PHP property type (if it's nullable, column will also be nullable). So that way you specify nullable in attribute only in places where for some reason you want different PHP and DB state, and you know what you are doing, because this will probably lead to error if not intended - for that form use-case it might be more typing, but it's opt-in in current state, default in 3.x will be as is.

I remember a lengthy discussion about this feature at the 2023 hackathon, although I fail to recall the details.

One thing I remember was that people may have a need for nullable properties to be able to bind form systems (like Symfony Forms) to entities, where fields may need to be null in code but not in DB… does that make sense?

Yes, that's basically the problem with nullability of properties. Unitil you persist it, an entity can basically represent an incomplete record. Imagine you create a new entity and then populate the properties one by one with then correct data. Symfony Forms is just one example where this is needed.

but inherited nullability is not forced, you can always override it with nullable attribute

Yeah, but this very argument can be brought up in favor of not inferring nullability. NOT NULL is not enforced, you can always override it with nullable: true. What ever default or inferring mechanism we chose, someone will complain about the default behavior.

Now, what you're proposing is a global flag that will affect all entities managed by an EM. This basically means that there's a single point of time where you can safely change this setting: When bootstrapping a new project. You cannot easily flip this switch on an existing project. You cannot iteratively switch your entities to the new way of inferring nullability. That will hurt the adoption of your feature.

You cannot use entities from external packages that have been written with the current defaults in mind. And a package that integrates with ORM by providing entities would need to test with both modes. And to be safe, such a package would need to do the one thing you want to avoid in your own codebase: Define nullability explicitly everywhere because the default might change depending on the EM's configuration.

I understand what you're trying to achieve here and that it would help you in your own project. But I believe that a global setting is the wrong way forward.

@derrabus so should I make it as a private constructor property in AttributeDriver and XmlDriver? This way it will be per-mapping and you can configure it in the mappings -> <name> -> inferNullabilityFromPHPType ORM section in symfony/nette extensions.

I see that 3.6 was released, so I changed the target, I could change also 3.6 to 3.7 in the docs I changed, but I'm pretty confident that it won't get into 3.7, since I'm usually waiting months for a reply and at the end I'll hear that people are using entities as data objects for symfony/forms as an argument against.

@derrabus so should I make it as a private constructor property in AttributeDriver and XmlDriver?

That would be better, but it still doesn't allow an iterative approach for switching from one mode to the other. But I don't really have a better idea. Sorry, I'd like to be more helpful. 😓

I see that 3.6 was released, so I changed the target

Perfect. Normally we do this ourselves, but we kinda forgot it this time. 🙈

but I'm pretty confident that it won't get into 3.7,

Maybe, we don't plan feature releases with fixed scopes.

since I'm usually waiting months for a reply and at the end I'll hear that people are using entities as data objects for symfony/forms as an argument against.

It's not an argument against your PR, it's a requirement we've had when implementing the current solution. That requirement is still valid and should not be neglected.

I understand your frustration, but please bear with us. The ORM project is not really well staffed and this change is not really our topmost priority because it replaces a solution that works for us.

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

Sorry for the delay, this time I was the one who went quiet for months.

Moved inferNullabilityFromPHPType from a global Configuration setting to a per-driver constructor parameter, as discussed with @derrabus. Each driver now sets the flag on ClassMetadata at the start of loadMetadataForClass(), so different mappings can use different modes.

Also prepared integration PRs for both frameworks and tested both on real apps:

• Symfony: Add support for inferNullabilityFromPHPType mapping option DoctrineBundle#2222
• Nette: Add support for inferNullabilityFromPHPType mapping option contributte/doctrine-orm#137

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

This pull request was closed due to inactivity.

Hello could this be reopen? If so, I could do a rebase and squash, but please could someone look at it in the next 3 months? If you don't want it just say so, so I know im not wasting my time here.