Skip to content
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

fix: [Validation] DotArrayFilter returns incorrect array when numeric index array is passed #8414

Closed
wants to merge 1 commit into from

Conversation

grimpirate
Copy link
Contributor

@grimpirate grimpirate commented Jan 15, 2024

Description
The use of array_merge_recursive rewrites numerically indexed values when attempting to retrieve validated data. Use of the array_merge_recursive_distinct function preserves their ordering. For an in-depth explanation and test harness.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@grimpirate grimpirate changed the title fix:Update DotArrayFilter.php fix: Update DotArrayFilter.php Jan 15, 2024
@kenjis
Copy link
Member

kenjis commented Jan 16, 2024

Thank you for this PR!

But some GitHub Action checks are not successful.
Please update your code to pass all checks.

See

Also we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works.
Please add test code in \CodeIgniter\Validation\DotArrayFilterTest https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Validation/DotArrayFilterTest.php.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

@kenjis kenjis changed the title fix: Update DotArrayFilter.php fix: DotArrayFilter returns incorrect array when numerically indexed array is passed Jan 16, 2024
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jan 16, 2024
@kenjis kenjis changed the title fix: DotArrayFilter returns incorrect array when numerically indexed array is passed fix: [Validation] DotArrayFilter returns incorrect array when numerically indexed array is passed Jan 16, 2024
@grimpirate
Copy link
Contributor Author

grimpirate commented Jan 16, 2024

I've provided a simplified unit test that fails when using array_merge_recursive but succeeds with the use of array_replace_recursive. Don't know that it covers every use case so if a more seasoned developer could consider the ramifications I would be appreciative. There appears to be a similar issue in Laravel.

@kenjis kenjis changed the title fix: [Validation] DotArrayFilter returns incorrect array when numerically indexed array is passed fix: [Validation] DotArrayFilter returns incorrect array when numeric index array is passed Jan 16, 2024
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first commit is completely removed. So squash the commits if you can.

The use of array_merge_recursive rewrites numerically indexed values when attempting to retrieve validated data. Use of the array_replace_recursive function preserves their ordering. Unit test demonstrates the failure of array_merge_recursive's use rather than array_replace_recursive
@kenjis
Copy link
Member

kenjis commented Jan 16, 2024

@grimpirate
Copy link
Contributor Author

There's no GPG associated with my account.

@kenjis
Copy link
Member

kenjis commented Jan 16, 2024

How did you sign the commits before squashing?

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

We cannot merge commits without signing. Thank you.

@grimpirate
Copy link
Contributor Author

I made the changes directly on the github platform, but I couldn't squash the commits unless I did it locally.

@grimpirate
Copy link
Contributor Author

I'll close this and open a new pull request without the squash.

@kenjis
Copy link
Member

kenjis commented Jan 17, 2024

@grimpirate Okay, no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants