added php 8.4 support and tests #379
Open
Conversation
This file contains hidden or 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
There was a problem hiding this comment.
Pull Request Overview
This PR adds PHP 8.4 support and related CI coverage, replaces legacy utf8_encode/utf8_decode calls with mb_convert_encoding, and tightens up type hints with nullable types across the codebase.
- Switch encoding functions to
mb_convert_encodingin both source and tests - Update method signatures to use nullable type hints for PHP 8.x compatibility
- Add PHP 8.4 job to the GitHub Actions workflow and bump
composer.jsonrequirement
Reviewed Changes
Copilot reviewed 29 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| composer.json | Added ^8.4 to supported PHP versions |
| .github/workflows/tests.yaml | Introduced php8-4 job for CI |
| src/Util/Json.php | Replaced utf8_encode with mb_convert_encoding |
| src/ShareUrl/Policy/DynamicPolicyBuilder.php | Made parameters nullable and renamed advanced identity method |
| tests/Util/JsonTest.php | Updated tests to use mb_convert_encoding instead of utf8_decode |
| tests/ShareUrl/Policy/DynamicPolicyBuilderTest.php | Updated test to call the renamed method |
Comments suppressed due to low confidence (2)
tests/ShareUrl/Policy/DynamicPolicyBuilderTest.php:722
- [nitpick] This abbreviated method name is inconsistent with other builder methods which use full words (e.g.,
withAdvancedIdentityProfileRequirements). Consider keeping the descriptive name for better clarity and API consistency.
->withAdvIdentityProfileReqs($advancedIdentityProfileSample)
src/ShareUrl/Policy/DynamicPolicyBuilder.php:414
- [nitpick] The method name
withAdvIdentityProfileReqsis an abbreviation of the originalwithAdvancedIdentityProfileRequirements. Abbreviations can reduce readability; consider restoring the full, descriptive name to maintain a clear and consistent API.
public function withAdvIdentityProfileReqs($advancedIdentityProfileRequirements): self
Comment on lines
+84
to
+102
| $latin1String = mb_convert_encoding('éàê', 'ISO-8859-1', 'UTF-8'); | ||
| $latin1Array = [ | ||
| mb_convert_encoding('éàê', 'ISO-8859-1', 'UTF-8'), | ||
| mb_convert_encoding('çî', 'ISO-8859-1', 'UTF-8') | ||
| ]; | ||
| $nestedLatin1Array = [ | ||
| mb_convert_encoding('éàê', 'ISO-8859-1', 'UTF-8'), | ||
| [ | ||
| mb_convert_encoding('çî', 'ISO-8859-1', 'UTF-8'), | ||
| mb_convert_encoding('üñ', 'ISO-8859-1', 'UTF-8') | ||
| ] | ||
| ]; | ||
|
|
||
| $latin1Object = new \stdClass(); | ||
| $latin1Object->property1 = utf8_decode('éàê'); | ||
| $latin1Object->property2 = utf8_decode('çî'); | ||
| $latin1Object->property1 = mb_convert_encoding('éàê', 'ISO-8859-1', 'UTF-8'); | ||
| $latin1Object->property2 = mb_convert_encoding('çî', 'ISO-8859-1', 'UTF-8'); | ||
|
|
||
| $nestedLatin1Object = new \stdClass(); | ||
| $nestedLatin1Object->property = utf8_decode('çî'); | ||
| $nestedLatin1Object->property = mb_convert_encoding('çî', 'ISO-8859-1', 'UTF-8'); |
There was a problem hiding this comment.
[nitpick] There are many repeated calls to mb_convert_encoding with the same parameters. Consider extracting a small helper function (e.g., latin1($str)) to DRY up these tests and improve readability.
Copilot uses AI. Check for mistakes.
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.
No description provided.