Reconsider enforcement of union types over the nullable ones

[morozov]

I do not believe that the coding standard should enforce the usage of union types over the nullable ones. From the original discussion in https://github.com/doctrine/orm/pull/9886#discussion_r917306253:

Personally, I believe arbitrary union types (e.g. getValue(sting $key): int|string|false) often reflect poor API design while nullable types are fine (e.g. findUser(int $id): ?User).

I think that enforcing a feature introduced primarily to support the existing poor design of the PHP's own API and the type system as a one-size-fits-all solution is not the right move.

[...]

IMO, union types are often poor because they make the consumer deal with a set of types instead of one by adding conditionals. Without pattern matching, it's error-prone since there's no guarantee that every consumer will handle each type from the union.

Specifically, in the PHP's standard library, union types go hand in hand with the APIs that are known as extremely error-prone, e.g. if (strpos($str, substr)) ...) or while ($value = $stmt->fetch(PDO::FETCH_COLUMN)) .... Union types are also needed to deal with array keys because there's no built-in support for maps in PHP and numeric array keys are converted to strings.

All the int|string types used in the code that fetches data from the database are because there's no support for unsigned integers and decimals in PHP. Otherwise, it would be possible to implement an API that returns a specific type.

Looking at the DBAL code, the only place I see where a union type is used deliberately is the expression builder where the methods accept strings or expressions but even there they could be split into two: one for stings, the other for expressions.

[derrabus]

The type declarations ?SomeType and SomeType|null are 100% equivalent and this rule enforces the latter. If we have multiple ways to express the same thing, it is perfectly reasonable to enforce one of the ways via a CS rule.

union types are often poor

A nullable type is a union. Your arguments apply to nullable types as they apply to any other union and I understand that avoiding unions is probably a good idea. However, I fail to see how this is relevant for our coding standard.

I don't think we should roll back.

[morozov]

Language shapes the way we think. I do not want the language (the coding standard) to force me into thinking in the broad categories of union types, I want to be able to use more expressive and specific nullable types, where necessary.

In my opinion, union types in PHP are a smell, and I do not want this smell to be enforced. Note, a smell doesn't mean that something is definitely wrong but it's often a sign that something is potentially wrong. As a reader of the code, I want to be able to spot a union without applying extra effort.

If we have multiple ways to express the same thing, it is perfectly reasonable to enforce one of the ways via a CS rule.

Following this logic, we should enforce the alphabetical order of class member declaration.

[greg0ire]

@morozov are you saying int|string and int|null are not equally bad, and that you want to spot the former more easily? In yes, can you please elaborate on why they are not equally bad?

[morozov]

Yes. int|null is better than int|string because as a set it has less capacity (null is just one distinct value) and often has the same semantics of "no value". The more specific the type is, the better.

The usage of NULL often naturally reflects the nature of the model being programmed. For instance getUser(): ?User naturally reads as "this method will return a user or no user". At the same time, the meaning of setParameter(int|string $parameter, ...) is not that clear.

[simPod]

I'm the author of the PR so I'm biased but 100% agree with @derrabus

For instance getUser(): ?User naturally reads as "this method will return a user or no user"

getUser(): User|null reads the same.

[greg0ire]

I agree that I don't feel the same amount of disgust when I see ?User than when I see User|Car, and that maybe this case deserves a special treatment because it's not the same level of bad, so I'd lean more on the side of ?

getUser(): User|null reads the same.

To me it reads like "this method will return a user or null" … in case of failure?