Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns the array_flip() function implementation with official PHP behavior by adding proper argument validation, type checking, and value type filtering. The previous implementation returned NULL for invalid inputs; the updated version throws appropriate exceptions (ArgumentCountError, TypeError) and warnings for unsupported value types.
Changes:
- Updated
array_flip()to throw ArgumentCountError for incorrect argument count and TypeError for non-array arguments - Added value type validation that emits warnings and skips entries with unsupported types (null, bool, float, array, object, resource), only accepting int and string values as keys
- Added comprehensive test coverage with integration tests for error conditions and smoke tests for value type handling
- Fixed test runner to return true from error handler to suppress default PHP error output
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ph7/hashmap.c | Updated ph7_hashmap_flip to validate arguments and value types per PHP spec |
| tests/phpt.php | Fixed handle_error to return true, preventing default error handler from running |
| tests/ph7/002-integration/function/array_flip/array_flip_missing_args.phpt | Added test for ArgumentCountError with no arguments |
| tests/ph7/002-integration/function/array_flip/array_flip_extra_args.phpt | Added test for ArgumentCountError with too many arguments |
| tests/ph7/002-integration/function/array_flip/array_flip_invalid_type.phpt | Added test for TypeError with string argument |
| tests/ph7/002-integration/function/array_flip/array_flip_invalid_type_obj.phpt | Added test for TypeError with object argument |
| tests/ph7/001-smoke/function/array_flip/array_flip_null.phpt | Added test for warning when value is null |
| tests/ph7/001-smoke/function/array_flip/array_flip_bool.phpt | Added test for warning when value is boolean |
| tests/ph7/001-smoke/function/array_flip/array_flip_float.phpt | Added test for warning when value is float |
| tests/ph7/001-smoke/function/array_flip/array_flip_arrayval.phpt | Added test for warning when value is array |
| tests/ph7/001-smoke/function/array_flip/array_flip_objectval.phpt | Added test for warning when value is object |
| tests/ph7/001-smoke/function/array_flip/array_flip.phpt | Updated test description and output format for duplicate value handling |
| tests/ph7/001-smoke/function/array_flip/array_flip_object.phpt | Removed obsolete test (replaced by integration test) |
| tests/ph7/001-smoke/function/array_flip/array_flip_invalid.phpt | Removed obsolete test (replaced by integration test) |
| tests/ph7/001-smoke/function/array_flip/array_flip_edge_cases.phpt | Removed obsolete test (replaced by new smoke tests) |
| tests/ph7/001-smoke/function/array_flip/array_flip_duplicate_values.phpt | Removed (functionality covered by updated array_flip.phpt) |
Comments suppressed due to low confidence (5)
tests/ph7/001-smoke/function/array_flip/array_flip_null.phpt:15
- The EXPECTF pattern only checks for the error message but doesn't verify the "PASS" output from line 12. Since match_expectf_pattern allows trailing content, this test would pass even if the code outputs "FAIL" instead of "PASS", as long as the error message appears. The EXPECTF should include both the error line and the PASS output on separate lines to ensure the test actually validates the correct behavior.
--EXPECTF--
Error [2]: array_flip(): Can only flip string and integer values, entry skipped in %s on line %d
tests/ph7/001-smoke/function/array_flip/array_flip_float.phpt:15
- The EXPECTF pattern only checks for the error message but doesn't verify the "PASS" output from line 12. Since match_expectf_pattern allows trailing content, this test would pass even if the code outputs "FAIL" instead of "PASS", as long as the error message appears. The EXPECTF should include both the error line and the PASS output on separate lines to ensure the test actually validates the correct behavior.
--EXPECTF--
Error [2]: array_flip(): Can only flip string and integer values, entry skipped in %s on line %d
--CLEAN--
tests/ph7/001-smoke/function/array_flip/array_flip_bool.phpt:15
- The EXPECTF pattern only checks for the error message but doesn't verify the "PASS" output from line 11. Since match_expectf_pattern allows trailing content, this test would pass even if the code outputs "FAIL" instead of "PASS", as long as the error message appears. The EXPECTF should include both the error line and the PASS output on separate lines to ensure the test actually validates the correct behavior.
--EXPECTF--
Error [2]: array_flip(): Can only flip string and integer values, entry skipped in %s on line %d
--CLEAN--
tests/ph7/001-smoke/function/array_flip/array_flip_arrayval.phpt:15
- The EXPECTF pattern only checks for the error message but doesn't verify the "PASS" output from line 11. Since match_expectf_pattern allows trailing content, this test would pass even if the code outputs "FAIL" instead of "PASS", as long as the error message appears. The EXPECTF should include both the error line and the PASS output on separate lines to ensure the test actually validates the correct behavior.
--EXPECTF--
Error [2]: array_flip(): Can only flip string and integer values, entry skipped in %s on line %d
--CLEAN--
tests/ph7/001-smoke/function/array_flip/array_flip_objectval.phpt:15
- The EXPECTF pattern only checks for the error message but doesn't verify the "PASS" output from line 12. Since match_expectf_pattern allows trailing content, this test would pass even if the code outputs "FAIL" instead of "PASS", as long as the error message appears. The EXPECTF should include both the error line and the PASS output on separate lines to ensure the test actually validates the correct behavior.
--EXPECTF--
Error [2]: array_flip(): Can only flip string and integer values, entry skipped in %s on line %d
Coverage
Details: https://github.com/alganet/PHL/actions/runs/22488358096 |
f12cde7 to
550b9b6
Compare
No description provided.