This repository has been archived by the owner on Dec 16, 2019. It is now read-only.
Fixed unnecessary nullable method signature BC breaks in PHP 7.1+ #835
Conversation
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
In PHP 7.1+, these resulted in the signatures becoming `?typehint`, which raises warnings when upgrading old code which overrides these methods. As it turns out, these parameters are not supposed to accept null at all (see the arginfo). These nullable flags are not necessary; it is sufficient to declare that only X parameters are needed. This will break code written for pthreads 3.1.7dev which gave null explicitly to these methods; however the fact that these methods accept null is not documented anywhere and I think it is a bug. This is backwards compatible with code which overrides these methods since parameter type widening allows this.
sirsnyder
approved these changes
Feb 20, 2018
|
Yes, it's undocumented and buggy behavior. I think these changes are ok, also with a BC break. |
|
@tpunt what's your opinion about the BC break? Ok, not ok? |
|
@sirsnyder The 3.1.6 release doesn't have these issues since it was only compatible with PHP 7.0 anyway. The bug only exists in versions of pthreads compatible with PHP 7.1 (3.1.7dev only), which since it doesn't have a release, these fixes are technically not a BC break anyway (it doesn't break the released public API). Also as discussed in #814 there are other BC breaks here which also haven't been covered in a new release, so this can be wrapped as a bugfix. |
|
I've pushed an additional fix to address the |
dktapps
referenced
this pull request
in pmmp/ext-pmmpthread
Feb 21, 2018
This updates the stub to match pthreads v3 properly, as per the arginfo of the extension. Some issues were discovered in the process (see #835 and #836), so method signatures have been adjusted to match the documentation where there are mismatches. The formatting in this file is very inconsistent, I went to great pains to avoid reformatting it to avoid polluting the diff.
|
Merged, thanks. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In PHP 7.1+, these resulted in the signatures becoming
?typehint, which raises warnings when upgrading old code which overrides these methods. As it turns out, these parameters are not supposed to accept null at all (see the implementation).These nullable flags are not necessary; it is sufficient to declare that only X parameters are needed.
This will break code written for pthreads 3.1.7dev which gave null explicitly to these methods; however the fact that these methods accept null is not documented anywhere and I think it is a bug.
This is backwards compatible with code which overrides these methods since parameter type widening allows this.