Utility\CaseInsensitiveDictionary: add dedicated unit tests + minor bug fix #558
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
jrfnl
force-pushed
the
feature/caseinsensitivedictionary-add-tests
branch
from
1d37d71
to
2faecbc
Compare
While this class was largely already tested via "user" classes, this now adds a set of dedicated tests specifically for this class.
jrfnl
force-pushed
the
feature/caseinsensitivedictionary-add-tests
branch
from
2faecbc
to
9adf84c
Compare
The use of `strtolower()` on non-string keys, is 1) not necessary and 2) had side-effects when non-string keys were used as those were then cast to string, which with the current set of tests would lead to the `false` test case overwritting the `null` test case and potentially "passing null to non-nullable" errors in PHP 8.1. While this class is intended to be used with requests headers, the class in itself is not limited to this use-case, so should function as per the specifications, independently of the use-case. Alternatively, a stricter check could be put in place to only allow string keys for the data captured in this class. That would potentially create a bigger breaking change though.
jrfnl
force-pushed
the
feature/caseinsensitivedictionary-add-tests
branch
from
9adf84c
to
0cc1e15
Compare
schlessera
approved these changes
Oct 11, 2021
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.
Utility\CaseInsensitiveDictionary: add dedicated unit tests
While this class was largely already tested via "user" classes, this now adds a set of dedicated tests specifically for this class.
Utility\CaseInsensitiveDictionary: minor bug fix
The use of
strtolower()on non-string keys, is 1) not necessary and 2) had side-effects when non-string keys were used as those were then cast to string, which with the current set of tests would lead to thefalsetest case overwriting thenulltest case and potentially "passing null to non-nullable" errors in PHP 8.1.While this class is intended to be used with requests headers, the class in itself is not limited to this use-case, so should function as per the specifications, independently of the use-case.
Alternatively, a stricter check could be put in place to only allow string keys for the data captured in this class. That would potentially create a bigger breaking change though.
Related to #497