[RFC] Read nullability from PHP type

[Rixafy]

Feature Request

Q	A
New Feature	yes
RFC	yes
BC Break	yes

I would like to have nullable columns according to my PHP nullable properties, when column property is nullable, column should be also nullable, if there is no nullable attribute that says otherwise, I checked my entities in multiple big projects, and I have 1:1 nullable database values with nullable PHP properties.

#[Column]
private ?string $content = null

This code should result in column being nullable in database, if there was declared #[Column(nullable: false)], column would not be nullable, but PHP variable can be, what is really bad practice, since entity can get into invalid state, and if it will be saved, application runs into an error. I thought about it, and I can't come up with one example where it's good to have nullable field in PHP, but not in database.

Previous discussions

This matter was discussed multiple times, once, this feature it was shipped into the 2.9.1 version, so some programmers (#8770, #8834) (me including) have removed nullable property from Column annotation, in version 2.9.2 it was reverted (#8732) and a BC break was introduced. I doubt there was many people that had different nullability in PHP.

Currently it's stated that it is not in 2.x because of backward compatibility (#8835), and main issue that caused rollback of the feature was this #8723 (and I think that only that one issue). And in that issue it was stated by @beberlei #8723 (comment) that you can look at it in ORM 3, what would be great, but then I asked about it recently, and got answer #9736 (comment)

No, nullability won't be deferred from property type declarations. This has been evaluated and discussed and we ultimately decided against it.

So, I'm curious based on what did you decide to not inherit nullability from PHP types?

Motivation

My vision is to have clean code, less bugs and less repeating declarations, ORM has been been going this way last couple of years.

For instance:

1. There are mapped default PHP types to default doctrine types, in most cases we need only #[Column] attribute.
2. Enums are supported and there is no need to declare anything (string, etc.), just enum type and #[Column], not even enumType, since it's inherited from PHP type
3. Same with embedded, we only need to declare #[Embedded]
4. We also don't need to declare targetEntity in ManyToOne relationships, it's inherited from PHP type.

Also it enforces entity valid state, because if PHP value could be null and if there was non-null column in database, after flushing the entity there will be a SQL error. So there is that.

And if someone want nullable type to be non-nullable in database, they can simply declare nullable: false in #[Column] attribute.

Another thing is, that happened to me a few times, that I totally forgot to declare nullable columns in annotations additionally, and I discovered it after I wanted to save entity, that PHP values could be nullable, but database columns could not.

Here is an example of that mentioned code where I needed to add additional nullable attribute and how bad it looks

And here is same code without that attribute nullability declarations

[beberlei]

The problems we face are:

• how do we "deprecate" the old functionality for users of ORM 2, so that they know what to fix in their mapping before upgrading to ORM 3.
• how to handle the case of ManyToOne and OneToOne which are nullable by default, however properties in PHP are not nullable by default. Should we change the behavior of both assocations to be non nullable by default?

It would be technically feasible to use null or not null as the default for nullable option and only override it when using the option explicitly. The problem why this was reverted ultimateily is, how do we get there from the ORM 2 behavior without f***ing everyone's existing code up.

[Rixafy]

Well, when someone updates from 2.0 to 3.0, BC breaks are expected, I bet that most people will have not a single line in migration, since now they are forced to declare nullability via attribute, it would mean that they have nullable property and non-nullable column in database, what would result in error when saving entity with null value, so they will have maybe few lines in migration and they check the entity properties, figure out only they are nullable, and therefore they add nullable: false or they realize this shouldn't be nullable in PHP or should be nullable in database.

In conclusion, that migration will probably help them discover bugs.

Another deprecating solution is to add deprecated yellow warnings when generating migrations or orm schema update, that there are entity properties that are nullable but database nullability doesn't match, so they should add nullable: false to entity attribute/annotation, because in 3.0, sql commands will be generated.

I think that ManyToOne and OneToOne should stay nullable by default for a while, that's something else and those are not regular column types but relations, that would be way too big BC break and I don't think users are ready for it, maybe it could be discussed and introduced in next version such as 4.0.

But on the other hand, if you make ManyToOne and OneToOne respect nullability, much more SQL would be generated in migration, therefore, there is not a chance that someone will overlook 1 SQL command, because there will be plenty of them and programmers will realize something has changed, so they visit ORM 3.0 docs where will be Upgrading from 2.x section and they figure out that php nullability is preferred.

I personally wouldn't have problem with relations being non-nullable, it would bring more consistency and less bugs, since now if I set somehow column in db to be null, ORM will probably scream that there is non-nullable variable and null was passed to that variable.

Another plus is that there can't be an empty value of "" in database relation, because it would result in foreign constraint fail so only valid value would be some id from another table.

[derrabus]

Well, when someone updates from 2.0 to 3.0, BC breaks are expected

Yes, but they have to be prepared. This means that there has to be some way to opt-in to the new behavior before it becomes mandatory with the major release. There are still applications out there using Zend Framework 1, Symfony 1 and/or Doctrine 1 because the upgrade path of each of those libraries to version 2 had been too difficult. The PHP ecosystem has learnt from its mistakes.

I bet that most people will have not a single line in migration,

You will lose that bet. A newly created entity does not necessarily need to be ready to be persited right away. I have seen quite a few codebases in the past where Entities were created like this:

$entity = (new MyEntity())
    ->setMandatoryFieldA('some value')
    ->setMandatoryFieldB('some value')
    ->setMandatoryFieldC('some value')
;

In that case, null is a valid property state for the entity class itself (because it may represent an incomplete record) although the corresponding database column does not accept null. And yes, such an incomplete entity cannot be persited, but that would be intentional in that case.

This is probably not an approach you would take and I respect that. But that does not render this approach invalid.

A special case of that are generated primary keys like this one:

#[Column, Id, GeneratedValue]
private ?int $id = null;

Here, the database column must not be nullable and an entity with $id === null can actually be persited.

So to conclude: even if we decide to guess nullability from the property type in 3.0, a proper deprecation layer has to be in place that notifies the developer that his current mapping configuration will trigger different behavior in 3.0. And here, the change is especially tricky because you want to change the semantics of the omission of the nullable property from well-defined defaults to a detection logic.

Another deprecating solution is to add deprecated yellow warnings when generating migrations or orm schema update, that there are entity properties that are nullable but database nullability doesn't match, so they should add nullable: false to entity attribute/annotation, because in 3.0, sql commands will be generated.

That might be a nice bonus, but using Doctrine Migrations is completely optional which is why our deprecation layer must not depend on it.

I think that ManyToOne and OneToOne should stay nullable by default for a while, that's something else and those are not regular column types but relations, that would be way too big BC break and I don't think users are ready for it, maybe it could be discussed and introduced in next version such as 4.0.

Taking into account the type of codebase I mentioned earlier, adding this kind of nullability guessing to relations is not particularly heavier than for regular columns.

[Rixafy]

That special cases with $id can be resolved with some exception for Id? That Id just can't be nullable in any scenario?

This means that there has to be some way to opt-in to the new behavior before it becomes mandatory with the major release.

That's sounds good, if there was option in configuration, for example preferPhpNullability, it could be added into 2.x with false as default value, and in ORM 3, the default value would be true, so if someone has trouble with upgrade, they can switch back to false. What would be mentioned in the docs in the Upgrade from 2.x section.

[Rixafy]

Hi @derrabus, do you think it's reasonable to add this at least as opt-in in configuration? It would help a lot to have cleaner entity properties, that's one of the few things why do I have something in JoinColumn or Column attribute.

Regarding that #[Id] attribute, I think we should make no exception, and inherit it as well (when configured), $id should not have nullable data type, because it should not be accessed before initialization. And if some reason it does have nullable PHP type, you can always change it with #[Column(nullable: false)].

[derrabus]

We can do that, yes. But someone would need to work on the feature and apparently nobody did that during the last three years.

[Rixafy]

We can do that, yes. But someone would need to work on the feature and apparently nobody did that during the last three years.

It has already been done, but it was reverted #8732, I tried to argue that we could have it at least opt in, so if it's possible, I'll just write same implementation when I'll find some free time, I'll just add configuration support and default option will be current behavior.