[Contracts] Add parameter type declarations to contracts

[derrabus]

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#32179
License	MIT
Doc PR	N/A

This PR proposes to create a php 7.2 version of the contracts that maintains BC with Symfony 4. The PR suggests to bump the contracts version to 1.2 2.0 on the master branch. We would still be able to maintain the contracts 1.1 branch on Symfony's 4.4 branch, should we need to patch the current contracts in the future.

This move would allow us to add parameter type declarations to existing contracts interfaces and make use of them in Symfony 5. Especially the Translation and EventDispatcher components benefit a lot from this bump, imho.

Contracts that will be added on the road to Symfony 6 wouldn't be restricted to the capabilities of php 7.1, which would be another benefit in my opinion.

¹ Test currently fail because the translator is called with null as translation key. That possibility should be deprecated imho.

[nicolas-grekas]

Test currently fail because the translator is called with null as translation key. That possibility should be deprecated imho.

We recently ensured that the implementation accepts null. Note that the interface can be string while the implementation (the trait) is ?string. That'd be perfectly fine to me. You could update the PR in that direction.

About the PR in general, we don't have the infrastructure to split symfony/symfony in two branches in symfony/contracts, as this PR would require.

But since v1.1 is going to be frozen as soon as 4.4.0 is out, we could bump contracts to 1.2 in December and work around the issue.

That'd work, right?

[derrabus]

Test currently fail because the translator is called with null as translation key. That possibility should be deprecated imho.

We recently ensured that the implementation accepts null. Note that the interface can be string while the implementation (the trait) is ?string. That'd be perfectly fine to me. You could update the PR in that direction.

All right, will do.

About the PR in general, we don't have the infrastructure to split symfony/symfony in two branches in symfony/contracts, as this PR would require.

I’m not familiar with the tooling you’re using to perform the subtree splits, but since all components are already split off the monorepo with multiple branches, I’m confident that this is possible.

But since v1.1 is going to be frozen as soon as 4.4.0 is out, we could bump contracts to 1.2 in December and work around the issue.

That'd work, right?

Certainly. But since the plan is to release 4.4.0 and 5.0.0 simultaneously, we would need to wait for 5.1 to merge this PR. Also, we wouldn’t be able to ship a 1.1 patch release anymore once we’ve bumped. If we could get the subtree split with two branches to work, that would be the the option I’d prefer.

[nicolas-grekas]

we wouldn’t be able to ship a 1.1 patch release anymore once we’ve bumped.

that's why we need to wait a bit, so we can freeze it completely :)

If we could get the subtree split with two branches to work, that would the the option I’d prefer.

we need a plan that would survive if we don't manage to have this tool. last time we checked, we decided it was too difficult.

I think we should bump to v2 btw (which means we should allow v2 in all composer.json that reference any contracts). That's how we do minor PHP bumps using Symfony policies. Relying on composer to block installing v1.2 because PHP 7.1 is used is a foot gun that hit our users before (remember symfony/intl when it shipped different versions based on the presence of ext-intl). I know others do it, but that's not a reason to do it too, IMHO.

[nicolas-grekas]

Oh btw, please borrow from nicolas-grekas@8cebfcd:)

[derrabus]

If we could get the subtree split with two branches to work, that would the the option I’d prefer.

we need a plan that would survive if we don't manage to have this tool. last time we checked, we decided it was too difficult.

I'll tinker a bit with subtree splits and get back to you. I'm not ready to give up just yet.

I think we should bump to v2 btw (which means we should allow v2 in all composer.json that reference any contracts).

That would mean, we would require ^1.1|^2 in Symfony 4.4 and ^2 in Symfony 5.x (whenever we feel ready to bump). Fine by me.

That's how we do minor PHP bumps using Symfony policies.

All changes in this PR including the php bump are non-breaking according to (my understanding of) SemVer. But it's of course okay to be stricter than SemVer, so 2.0 it is. 😃

[derrabus]

See #33500 for allowing Contracts 2 in Symfony 4.4.

[derrabus]

I've cherry-picked #33500 into this PR and switched all contracts to 2.0.

[derrabus]

I managed to split off a contracts repository with a 1.1 branch (from #33500) and a master branch (from this PR): https://github.com/derrabus/symfony-contracts-split

mkdir symfony-contracts
cd symfony-contracts
git init --bare
cd ../symfony
git checkout improvement/allow-contracts-2
git subtree split --prefix=src/Symfony/Contracts -b contracts-1.1
git push ../symfony-contracts contracts-1.1:1.1
git checkout improvement/typed-contracts
git subtree split --prefix=src/Symfony/Contracts -b contracts-master
git push ../symfony-contracts contracts-master:master

Your current tooling is a black box for me, but as far as I can tell, the master of the symfony/contracts repository is split off of the 4.4 branch of symfony/symfony. It shouldn't be too hard to set up 2 branches for the split. Suggested mapping:

symfony/symfony	symfony/contracts
4.4	1.1 or 1.x
master	master

[nicolas-grekas]

@derrabus could you please rebase this PR and ensure it's ready? It might be time to do it.

[derrabus]

Rebase done.

[nicolas-grekas]

I've rebased the PR to relaunch tests. Just one question.

[nicolas-grekas]

Before merging, we should tag v1.1.8 of symfony/contracts and symfony/http-client-contracts
And just after merging, we should tag v2.0.0 of symfony/contracts and symfony/*-contracts

[fabpot]

Thank you @derrabus.

[Tobion]

we should tag v2.0.0 of symfony/contracts

Do we still want to publish the contract meta package at all? Wouldn't it make sense to not tag it anymore just as it was planned to not tag v5 of symfony/symfony?

[nicolas-grekas]

I think we still have to publish the metapackage for our CI at least. Not tagging 5.0/2.0 creates burden on our side we don't have much time to deal with IMHO.

[derrabus]

we should tag v2.0.0

We could go with a beta first and tag v2.0.0 together with Symfony 5.0.0. Maybe we find an issue during the Symfony 4.4/5.0 beta test that we want to fix before tagging a final 2.0.0 on the contracts.

I think we still have to publish the metapackage for our CI at least.

Should we add a conflict rule for symfony/contracts to the skeleton then, like we have it for symfony/symfony?

https://github.com/symfony/skeleton/blob/cd58faa216e3a667efd03aecfdecc7296de38850/composer.json#L56-L58

[derrabus]

Tests on 4.4 are broken now because we still allow symfony/contracts v2 on symfony/symfony 4.4 but broke compatibility on the event dispatcher. Maybe it's not a bad idea after all to drop the metapacke in CI?

[nicolas-grekas]

We should unallow 2 from 4.4. Dropping the metapackage would break master then. And we (I) have better to do than this...

[derrabus]

#34307