Enable strict types

[greg0ire]

It is in our coding standard, and doing so is not a breaking change.

[ostrolucky]

Does it at least reduce phpstan errors? I'm not much of a fan of this change, as it conflicts with Symfony CS and doesn't really bring us much. On the other hand it makes code more brittle and gives us more work of having to juggle things between types, while PHP could automatically for us.

[greg0ire]

It does not reduce PHPStan errors, even at max level.

it conflicts with Symfony CS

OK, but so what? This repository is in the doctrine organization and is checked with PHPCodeSniffer, using the Doctrine coding standard. I don't think we should strive to be close to Symfony CS here, which full disclosure, I don't like for several reasons.

On the other hand it makes code more brittle and gives us more work of having to juggle things between types, while PHP could automatically for us.

The only thing I've had to change is to do some string casts. Benefiting from PHP automatic type casts is something I think is OK in a project, but in a library, I think it's more important to avoid silly type errors, even if it means slightly more ugly code.

Finally since it's in our standard, that's what we do in all our other projects, including other Symfony bundles like the migrations or fixtures bundle. I don't see why this bundle should get a special treatment.

[ostrolucky]

I'm approving this, because I don't think it's worth it to make a fuss about such change. I am not a fan personally but at the same time it's not such a big deal that I want to block this. However, I want to remind certain things:

1. This is a major symfony bundle. It's sitting between symfony and doctrine. Even supported PHP versions are mirrored to what Symfony LTS supports. Compromises from both sides are important here. It's not like you implied that repo is in doctrine organization so we do whatever doctrine wants. By the way this is why I didn't want to eagerly start 3.x branch. There are dependencies in Symfony to things we deprecated long time ago (eg. %.class params) that we cannot drop before Symfony stops using them. Such change is unlikely to happen in Symfony LTS and we need to support it. So it will take years until we can release 3.x (so that we can continue supporting LTS), unless we maintain both 2.x and 3.x like last time, which I would really like to avoid. On the other hand, maybe existence of 3.x having those things removed will push Symfony to stop using them?
2. This is a risky change and our CI is not perfect, like we saw in #1968. Whole point of strict types is that it will blow up more often than non-strict.

[greg0ire]

There are dependencies in Symfony to things we deprecated long time ago (eg. %.class params) that we cannot drop before Symfony stops using them

Ok, so either we drop support for 6.4 on 3.x, or we keep the %.class, is that it? I think both are fine, the important part for this 3.x release IMO is more to be aligned with ORM 4, and for ORM 4, the important thing is to require PHP 8.4 so that native lazy objects are available. We can totally postpone the removal of %.class to 4.x if you feel it's better.

[SenseException]

Then the target branch is supposed to be 3.0.x, right?

[greg0ire]

🤔 no… I mean, there is no breaking change here, is there?

Strict typing applies to function calls made from within the file with strict typing enabled, not to the functions declared within that file. If a file without strict typing enabled makes a call to a function that was defined in a file with strict typing, the caller's preference (coercive typing) will be respected, and the value will be coerced.

(from https://www.php.net/manual/en/language.types.declarations.php)