php-fig / container

MIT License
9.93k stars 52 forks source link

Add return type hints #28

Closed weierophinney closed 3 years ago

weierophinney commented 4 years ago

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 a v3v2 release. No other methods were eligible, as they specified mixed.

See https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/

weierophinney commented 4 years ago

This patch will require a 3.0.02.0.0 release.

weierophinney commented 4 years ago

Ping @moufmouf — are you okay with this?

weierophinney commented 3 years ago

@moufmouf Now that #27 is merged, we need a release of 1.1.0, and then we can merge this PR and do a 2.0.0 release. My understanding is that you need to do this, as editor of the spec, and/or you need to specify a new editor/proxy (I think you mentioned @mnapoli at some point?)

moufmouf commented 3 years ago

@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 ContainerExceptionInterface extends Throwable. This would close #21 .

Not sure if this was discussed on the mailing list earlier. What do you think?

weierophinney commented 3 years ago

@moufmouf and @mnapoli Makes sense; go for it!

Jean85 commented 3 years ago

@weierophinney gentle reminder that we need a CC vote to push this trough, as explained in https://github.com/php-fig/container/pull/27#issuecomment-707549684

weierophinney commented 3 years ago

@Jean85 Ah, right...

And that's up to the editors... paging @moufmouf and @mnapoli ...

Jean85 commented 3 years ago

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.

mnapoli commented 3 years ago

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.)

weierophinney commented 3 years ago

@XedinUnknown —

This is a BC-breaking change, as descendant signature must be identical, but it will not be because it would be missing the typehint, @weierophinney. Therefore, this should be in a new major version. Besides the fact that a previously supported PHP version was dropped.

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 preparation for a v2 release

In other words, the intention for this patch is for a new major version, which allows for BC breaks!

mindplay-dk commented 3 years ago

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? 😕

mnapoli commented 3 years ago

@mindplay-dk please read https://www.php-fig.org/bylaws/psr-evolution/ There is a smooth upgrade path for the community.

Jean85 commented 3 years ago

@mindplay-dk TL;DR of that bylaw: you can support ^1.1|^2.0 and be fine.

Or even better ^1.1.1|^2.0.1 to push your users away from issue in #30.

mindplay-dk commented 3 years ago

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?

Jean85 commented 3 years ago

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...

XedinUnknown commented 3 years ago

@weierophinney, the commit that I commented on is part of 1.1.0, and that's a non-BC-breaking release. But also I concede that this isn't actually a BC break. Due to the PHP bug, it was a hassle, but it's done now, so no problem. Thanks!