[Contracts] Add parameter type declarations to contracts #33497
Conversation
We recently ensured that the implementation accepts null. Note that the interface can be 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? |
All right, will do.
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.
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. |
that's why we need to wait a bit, so we can freeze it completely :)
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. |
|
Oh btw, please borrow from nicolas-grekas@8cebfcd:) |
f0e7df8 to
f5ae3ab
Compare
I'll tinker a bit with subtree splits and get back to you. I'm not ready to give up just yet.
That would mean, we would require
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. 😃 |
|
See #33500 for allowing Contracts 2 in Symfony 4.4. |
f5ae3ab to
ee3917d
Compare
ee3917d to
8c8188f
Compare
|
I've cherry-picked #33500 into this PR and switched all contracts to 2.0. |
c046917 to
d65ea8f
Compare
|
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:masterYour current tooling is a black box for me, but as far as I can tell, the
|
7df6c03 to
62f926d
Compare
This PR was merged into the 4.4 branch. Discussion ---------- Allow version 2 of the contracts package | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | prepares #33497 | License | MIT | Doc PR | N/A The plan is to release a version 2 of the contracts package that will require php 7.2 but remains compatible to Symfony 4. Commits ------- a1ee320 Allow version 2 of the contracts package.
62f926d to
a2fcebc
Compare
a2fcebc to
d71e91d
Compare
|
@derrabus could you please rebase this PR and ensure it's ready? It might be time to do it. |
d71e91d to
b061af0
Compare
|
Rebase done. |
b061af0 to
bf515e1
Compare
src/Symfony/Contracts/EventDispatcher/EventDispatcherInterface.php
Outdated
Show resolved
Hide resolved
…contracts (derrabus) This PR was merged into the 5.0-dev branch. Discussion ---------- [Contracts] Add parameter type declarations to contracts | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#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. ~~<sup>1</sup> Test currently fail because the translator is called with `null` as translation key. That possibility should be deprecated imho.~~ Commits ------- bf515e1 Add parameter type declarations to contracts.
646d954 to
55b6cf0
Compare
55b6cf0 to
4de3773
Compare
|
Thank you @derrabus. |
…ts (derrabus) This PR was merged into the 5.0-dev branch. Discussion ---------- [Contracts] Add parameter type declarations to contracts | 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. ~~<sup>1</sup> Test currently fail because the translator is called with `null` as translation key. That possibility should be deprecated imho.~~ Commits ------- 4de3773 Add parameter type declarations to 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? |
|
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. |
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.
Should we add a conflict rule for |
|
Tests on 4.4 are broken now because we still allow |
|
We should unallow 2 from 4.4. Dropping the metapackage would break master then. And we (I) have better to do than this... |
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.22.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.
1 Test currently fail because the translator is called withnullas translation key. That possibility should be deprecated imho.