-
Notifications
You must be signed in to change notification settings - Fork 501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QA: stricter type adherence #779
Merged
Merged
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
The `IdnaEncoder::digit_to_char()` method expects an `int`, not a float, for the `$digit` parameter. Same goes for the PHP native `substr()` function called from within the `IdnaEncoder::digit_to_char()` method to which the `$digit` parameter. This ensures that the values passed to `IdnaEncoder::digit_to_char()`, which due to the use of `%` and `floor` are typed as `float`, will now contain the correct `int` type. Note: no tests are added for this as the existing tests cover this already, but don't show the issue unless `strict_types` mode is turned on.
The PHP native `substr_replace()` function expects an integer for the `$offset` parameter, however, the `strrpos()` function used to determine the _offset_ will return an integer if the `$needle` is found, but `false` when the `$needle` doesn't exist in the `$haystack`. This tweaks the code to allow for that and ensures that the `substr_replace()` function will always receive an integer. Note: no tests are added for this as the existing tests cover this already, but don't show the issue unless `strict_types` mode is turned on.
The PHP native `strpos()` function expects an integer for the `$offset` parameter, however, the `strpos()` function used to determine the _offset_ will return an integer if the `$needle` is found, but `false` when the `$needle` doesn't exist in the `$haystack`. This tweaks the code to allow for that and ensures that the `strpos()` function will always receive an integer. Note: no tests are added for this as the existing tests cover this already, but don't show the issue unless `strict_types` mode is turned on.
A cookie will either be a key/value pair or just a plain value (with an implicit integer key). The `Cookie::normalize_attribute()` method normalized the value based on a string based `key` and otherwise returns the `value` unchanged, so this function call can be bypassed when the cookie is just a plain value without a key as it won't have any effect anyway. This prevents a call to the PHP native `strtolower()` function in the `Cookie::normalize_attribute()` method from passing an integer where a string is expected. Note: no tests are added for this as the existing tests cover this already, but don't show the issue unless `strict_types` mode is turned on.
An HTTP header will either be a key/value pair or just a plain value (with an implicit integer key). This minor tweak prevents a call to the PHP native `strtolower()` function being passed an integer where a string is expected. Note: no tests are added for this as the existing tests cover this already, but don't show the issue unless `strict_types` mode is turned on.
The `$request_path` parameter is expected to be a `string`, but in effect, all scalars are accepted, as well as stringable objects. While this is fine for the code in the first half of the function, the second half strongly relies on PHP type juggling as the type is treated as a string as of that point. To make this explicit and ensure that both the `strlen()` as well as the `substr()` functions will always receive the correct type, a type casts is being added. Note: no tests are added for this as the existing tests cover this already, but don't show the issue unless `strict_types` mode is turned on.
schlessera
approved these changes
Dec 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Introduction
This PR is the outcome of an experiment to turn on
strict_types
for this code base (which can not happen for real until support for PHP 5.6 has been dropped).Running the tests over the code base with
strict_types
turned on, initially yielded 646 tests erroring out and 156 tests failing (and three extra tests being skipped).The below commits fix the majority of these, save for 6 test errors.
These last 6 test errors are
TypeError
s for PHP native functions which expect astring
, but are receiving a stringable object, which for all intends and purposes is perfectly valid.If and when Requests would move to enabling
strict_types
for real, those last six issues can be solved by adding a(string)
cast in 3 places in the code base.Details of the last siẋ errors (fold out)
Note: no tests are added for any of these changes as the existing tests cover them already, but don't show the issue unless
strict_types
mode is turned on.Commits
QA | IdnaEncoder::punycode_encode(): pass the correct type
The
IdnaEncoder::digit_to_char()
method expects anint
, not a float, for the$digit
parameter. Same goes for the PHP nativesubstr()
function called from within theIdnaEncoder::digit_to_char()
method to which the$digit
parameter.This ensures that the values passed to
IdnaEncoder::digit_to_char()
, which due to the use of%
andfloor
are typed asfloat
, will now contain the correctint
type.QA | Iri::remove_dot_segments(): pass the correct type
The PHP native
substr_replace()
function expects an integer for the$offset
parameter, however, thestrrpos()
function used to determine the offset will return an integer if the$needle
is found, butfalse
when the$needle
doesn't exist in the$haystack
.This tweaks the code to allow for that and ensures that the
substr_replace()
function will always receive an integer.QA | Iri::set_authority(): pass the correct type
The PHP native
strpos()
function expects an integer for the$offset
parameter, however, thestrpos()
function used to determine the offset will return an integer if the$needle
is found, butfalse
when the$needle
doesn't exist in the$haystack
.This tweaks the code to allow for that and ensures that the
strpos()
function will always receive an integer.QA | Cookie::normalize(): pass the correct type
A cookie will either be a key/value pair or just a plain value (with an implicit integer key).
The
Cookie::normalize_attribute()
method normalized the value based on a string basedkey
and otherwise returns thevalue
unchanged, so this function call can be bypassed when the cookie is just a plain value without a key as it won't have any effect anyway.This prevents a call to the PHP native
strtolower()
function in theCookie::normalize_attribute()
method from passing an integer where a string is expected.QA | Headers::getValues(): pass the correct type
An HTTP header will either be a key/value pair or just a plain value (with an implicit integer key).
This minor tweak prevents a call to the PHP native
strtolower()
function being passed an integer where a string is expected.QA | Cookie::path_matches(): pass the correct type
The
$request_path
parameter is expected to be astring
, but in effect, all scalars are accepted, as well as stringable objects.While this is fine for the code in the first half of the function, the second half strongly relies on PHP type juggling as the type is treated as a string as of that point.
To make this explicit and ensure that both the
strlen()
as well as thesubstr()
functions will always receive the correct type, a type casts is being added.