Add return type hints #28
Conversation
This patch bumps the minimum supported PHP version to 7.2 and adds parameter typehints to ContainerInterface, as the first step towards adding explicit typehints based on the specification. See https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/
This patch adds a return type hint to `ContainerInterface::has()`, per the already documented specification, in preparation for a v3 release. No other methods were eligible, as they specified `mixed`. See https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/ Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
|
This patch will require a |
|
Ping @moufmouf — are you okay with this? |
I'd misread the original by-laws around typehint addition to require two new major versions, when it's just a minor version and a major version.
|
@weierophinney : @mnapoli and I are both co-editor (I think PSR-11 is the only PSR that has co-editors and it is probably not possible anymore due to the new bylaws). Before we tag 1.1.0, there is another improvement we could do. Since v1.1 targets PHP 7.2+, we could make sure Not sure if this was discussed on the mailing list earlier. What do you think? |
|
@weierophinney gentle reminder that we need a CC vote to push this trough, as explained in #27 (comment) |
|
Vote is a blocker only for tagging, you could/should do all the code work up front, but you also need a PR ready with the needed amendments to the PSR (which should reference the code), which is basically what the CC should vote on. |
|
Hi! On my end, I unfortunately don't have the bandwidth right now for the whole vote, but I'm able to tag. So I'll let this discussion continue, but if you need someone to tag a release at some point, let me know! (and for what it's worth I'm completely 👍 with extending throwable.) |
| public function get($id); | ||
| public function get(string $id); |
There was a problem hiding this comment.
Should we require PHP 8 here so we can include a mixed return? I'm tempted to say yes, although admittedly mixed is not the most descriptive type.
There was a problem hiding this comment.
We'll be able to get a stepped migration from users still on PHP 7 to users on PHP 8 if we omit it, which, as somebody looking at updating a library already, is going to be useful. Otherwise, libraries cannot even look at adopting it until PHP 8 is their minimum supported version.
There was a problem hiding this comment.
Maybe this qualifies for a follow-up 2.1 release, just bumping to 8.0 and adding that mixed return?
When I first saw this and commented last night, I was unaware the commit was part of this particular PR. Please READ THE PATCH SUMMARY:
In other words, the intention for this patch is for a new major version, which allows for BC breaks! |
Not sure why merting #28 did not pick this up...
|
So what happened to your reservations about "splitting the PHP community"? Upgrading to this is going to be a ginormous clusterfudge for the entire community - anyone using more than one package with a dependency on this interface is in trouble, more so if they're using other packages that depend on it. Until literally every project and package moves to version 2 of this, package availability will be split down the middle, locking people out of updates, or forcing maintainers to maintain two release lines in parallel. With version 3.0 of my own DI container being almost ready for release, I'm now torn between locking out existing users or blocking their update path to other libraries. Or maintaining parallel 3.x and 4.x release, with the 4.x line using version 2 of this interface. Is a simple, non-functional change really worth this kind of havoc? 😕 |
|
@mindplay-dk please read https://www.php-fig.org/bylaws/psr-evolution/ There is a smooth upgrade path for the community. |
|
@mindplay-dk TL;DR of that bylaw: you can support Or even better |
|
Ah, so my package should support both versions perpetually. Got it. I was assuming one would eventually move away from 1.x and require 2.x, but I don't suppose there's any reason to do that ever? |
|
You can require 2 only, no one is stopping you, the point is requiring a smooth upgrade path, so you must be in the position to support at least two contiguous version from two different majors. The bylaw is written in a generic way, who knows if in the future we will have a 3.0... |
|
@weierophinney, the commit that I commented on is part of |
This patch builds on #27 and requires a release of
2.0.01.1.0 before merging.The patch adds a return type hint to
ContainerInterface::has(), per the already documented specification, in preparation for av3v2 release. No other methods were eligible, as they specifiedmixed.See https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/