-
Notifications
You must be signed in to change notification settings - Fork 53
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: Adds parameter typehints to ContainerInterface
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/
- Loading branch information
1 parent
fc1bc36
commit 6c2bc7f
Showing
2 changed files
with
6 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weierophinney, you are formally right. However, this is breaking stuff quite a bit. IMHO, if something that was supported is no longer supported, that's a BC break. But I understand the point, thanks.
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XedinUnknown Composer protects you from upgrading to an incompatible version.
And this is a minor bump to leave space for patches on 1.0.x because 1.0 will stick around, it's not going away, and since it's a PSR we're absolutely not dropping support for a long time.
Also, as pointed out times and times again by Ocramius, bumping requirements is not a BC for SemVer: Ocramius/PackageVersions#105 (comment)
If something brakes on your side due to something like this, it's because you're doing something wrong, because you MUST run
composer update
only in production-compatible systems: Ocramius/PackageVersions#105 (comment)6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am running
composer update
, and I depend on^1.0
. This means that I should not be getting breaking updates. But somehow I got breaking changes fromdev-master
, which is even stranger now that I see the alias changed to2.0.x
. 🤔Just FYI: my problem is not with PHP versions; it is with signatures. In my project where I depend on
^1.0
, as soon as I installed on PHP 7.2 I got the new signatures with typehints, and this broke the stuff. I'm trying to find the CI log for this, but no luck yet somehow...6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that you're running with
"minimum-stability": "dev"
.You can't expect that nothing breaks if you do.
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always thought that if I'm running with
"minimum-stability": "dev"
, and depending on SemVer to not break BC (although I understand that SemVer's notion of BC relates only to the API and not dep constraints), then I should only get BC-breaking stuff it the break was unintentional.6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On master it's
1.1.x-dev
:container/composer.json
Line 24 in 381524e
That's why you got it, but it shouldn't break anything to you if you're on 7.2+. If you're not, you used
--ignore-platform-reqs
, which basically screws with all the safeguards of Composer.6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I certainly do not use
--ignore-platform-reqs
. What is happening is that I have an interface that extendsContainerInterface
, and when I implement it, the signatures are incompatible - even on PHP 7.4. I found the log, and you can see it here.6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a bit of time to try to find what was appending here and it looks like if you define an interface that has the same method than the
ContainerInterface
but without the type and then create an interface that extends both interface it doesn't work, even on PHP 7.4 : https://3v4l.org/INCMdThis looks like a bug in PHP since switching the interface order works without problems.
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvasseur, thank you very much, kind sir!
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug reference: #67270, #76361, #80785. Gawd, I hate the PHP issue tracker, because it's impossible to just find what you need there. Spent a good 20 minutes trying different search terms.
There is an opinion that this is nevertheless not a bug, because it doesn't break any spec that says that inheritance order should be irrelevant. I don't care bug or not, and I recognize that we should know this behaviour and to work around it. It could be useful, however, if PHP had some info on this available - like somewhere here.
6c2bc7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
5fe95a7
(#28). Thanks a lot for your support!