Add support for inherited nullability from PHP

[rixafy]

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->setInferNullabilityFromPHPType(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.

[derrabus]

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

[rixafy]

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]

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]

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]

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]

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]

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?

[rixafy]

I need some time to answer, this has a few internal implications that could prevent this

Can I somehow help with that? Are those implications related to the virtual creation of join column? I'm already using this PR on a few projects, so far I didn't have any problems, it completely eliminated need for nullable definition, before that I always forgot to define some property as nullable (but at least phpstan reported it).

[rixafy]

Hello, just a friendly reminder since it's been some time, it would be really awesome if this could get to 3.4 release.

[beberlei]

Related issues:

• [GH-8723] Remove use of nullability to automatically detect nullable status #8732
• Move JoinColumn nullability check to drivers #8726
• Upgrade from 2.8.x to 2.9.1: possible BC break for nullable behaviour #8723

[beberlei]

I see two problems with this:

1. For regular properties, for post commit id generators, you need to leave the id property nullable at least until the first flush, because this code execution order requires it to be null:

$entity = new Entity();

// in a different method working on both persistent and non persistent entities.
if ($entity->getId()) {
   // already in db
}

So public ?int $id might cause schema to set the ID nullable though that is only from the code perspective not the database one.

2. For assocations, inheritance, the column must be nullable in some commit order cases of bi-directional one to one or one to many. See the extra updates in UnitOfWork and persisters. This is a temporary workaround that is fixed by issuing the calls in a transaction, but for some time a column is null. This can only be detected by looking at multiple entity metadatas together and how their assocations relate, so its not possible to infer this in ClassMetadata::completeAssocationMapping.

Therefore I would like to see this limited to only fields and not to associations and only to set nullable=true if allowsNull=true. And there must be a special case for 'id' fields where it does not infer this automatically.

[greg0ire]

it would be really awesome if this could get to 3.4 release.

Sorry, I'm going to release 3.4. Don't get disheartened though, we have already stuff we want to have in 3.5, so it should be released soon after 3.4.

[rixafy]

1. For regular properties, for post commit id generators, you need to leave the id property nullable at least until the first flush, because this code execution order requires it to be null:

Well, if you don't access that property, it does not need to be null, I'm using auto-increment and I have all my IDs non-nullable in code and in database, and when I was using UUID, I also didn't have it nullable, because I configured it when constructing entity.

If such a case exists and someone want to access uninitialized property, nullability can be always set:

#[Id, Column(nullable: false)]
private ?int $id;

So I would rather keep it consistent and inherit that nullability, or maybe I don't quite get why would it need to be nullable in code in the first place, I have never had such case. But I guess it's not a problem to make exception for Id attribute, I just want to avoid inconsistencies.

2. For assocations, inheritance, the column must be nullable in some commit order cases of bi-directional one to one or one to many. See the extra updates in UnitOfWork and persisters. This is a temporary workaround that is fixed by issuing the calls in a transaction, but for some time a column is null. This can only be detected by looking at multiple entity metadatas together and how their assocations relate, so its not possible to infer this in ClassMetadata::completeAssocationMapping.

Can you give me example what code could cause the error? I have multiple one to one associations and so far I didn't encountered an error, some of them are nullable and some of them are not, but I have only 1 bidirectional.

I would argue that it can also happen now when someone declares JoinColumn(nullable: false), and it can always be overwritten with JoinColumn attribute for such cases, inherited nullability doesn't mean it's forced, it can always be overwritten, but it saves a lot of boilerplate code so we don't declare nullability twice. If someone is using phpstan-doctrine, it would report an error if association had different nullability between code and db, so people are pushed into keeping it either nullable in code or declare nullable: false in JoinColumn.

It's also made as opt-in feature via $config->setInferNullabilityFromPHPType(true), it makes no sense to me why it should only work for fields and not associations, I found most inconsistencies in associations, that I had somehow null values in database and in php that property type wasn't nullable.

Sorry, I'm going to release 3.4. Don't get disheartened though, we have already stuff we want to have in 3.5, so it should be released soon after 3.4.

No problem, just let me know if there will be another branch so I can rebase.

[greg0ire]

There is another branch already, 3.5.x ;)

[rixafy]

@beberlei I have implemented an exception for ID columns (also covered by tests), even though I don't think it's a good idea to introduce inconsistent behavior and encourage having $id as nullable, and based on that, checking if the entity is in the database. For such cases, I would encourage users to declare nullable: false, but I want this PR to be merged.

Also, have you changed your mind about this feature in associations? You pointed out some bidirectional issues, but I think such cases could be handled with nullable: true. It still saves a lot of boilerplate from having to define nullable: false everywhere, making it 1:1 with PHP and reducing the chance of errors.

If you're still against it, I would like to suggest two separate booleans: one for regular fields and one for association columns, so it would look like setInferNullabilityFromPHPType(bool $fields, bool $associations), or maybe with even 3rd boolean like $ignoreIdColumns so it's flexible for everyone, I don't mind that Id, because I doubt someone would have it nullable in database, but associations are very important to me, since I would get rid of almost every JoinColumn.

[derrabus]

Well, if you don't access that property, it does not need to be null, I'm using auto-increment and I have all my IDs non-nullable in code and in database, and when I was using UUID, I also didn't have it nullable, because I configured it when constructing entity.

The uninitialized state is a footgun. Benjamin is right, we have to allow auto-increment ID fields to be nullable without mapping them to nullable fields in the schema.

[rixafy]

The uninitialized state is a footgun. Benjamin is right, we have to allow auto-increment ID fields to be nullable without mapping them to nullable fields in the schema.

Ok, just to be clear, it was allowed, but it had to be overwritten via nullable: false in Column attribute, after my last change the auto inheritance for Id column is disabled completely.

[rixafy]

@greg0ire @SenseException I'm going to do a rebase, but I'm unsure how to proceed because of possible BC break related to #12108, there was a short discussion satarted by @morozov #12108 (comment), similar thing was mentioned by @derrabus here, so I added new property $nullableSet that indicates if property was changed by user - 50e693d#diff-21e3020b36bd7b9e715c776d5323344e0da87d73358825c3b74ceeeeda6660a8R14 and this way I bypassed the possible BC break.

I don't know if I should keep this $nullableSet in JoinColumnProperties (I would need to refactor change from #12108 so it's still true and not null + some changes elsewhere to rely on $nullableSet) and Column (where it's still untouched), or I should also make property $nullable nullable in Column and remove $nullabbleSet so it's consistent with this new change what that other PR introduced.

With $nullableSet we guarantee no BC break if someone is using the attribute classes somewhere in their codebase, but with null default value it's more clean, especially now when we could have inherited nullability by default in 4.0.. or we could change that public property to nullable and remove $nullableSet in 4.0?

Btw. could this be please added to 3.6.0 milestone so it's not forgotten?

[github-actions]

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.

[rixafy]

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.

[rixafy]

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

[mpdude]

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?

[rixafy]

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.

[derrabus]

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.

[rixafy]

@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.