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

Arrayhelper filter support more than 2 levels #17830

Merged

Conversation

rhertogh
Copy link
Contributor

Q A
Is bugfix? ✔️
New feature?
Breaks BC? ✔️(But only if ArrayHelper::filter was used in an unexpected way)
Tests pass? ✔️
Fixed issues #17829

@rhertogh rhertogh requested a review from samdark January 24, 2020 17:51
…ilter` when passing a filter with more than 2 levels
@samdark samdark added this to the 2.0.33 milestone Jan 26, 2020
@samdark samdark added type:bug Bug status:code review The pull request needs review. labels Jan 26, 2020
@samdark samdark requested a review from a team January 26, 2020 22:26

//We've found a value now let's insert it
$resultNode = &$result;
foreach ($keys as $key) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you merge this loop with previous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, let's say we have a filter 'A.B' and the source does have key A but that level does not have key B in it, the resulting array would be set [A => []] while it should return [].

Copy link
Member

Choose a reason for hiding this comment

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

Are tests fix this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the test would fail at line 1336 $this->assertEquals(ArrayHelper::filter($array, ['A.X']), []);.

framework/helpers/BaseArrayHelper.php Outdated Show resolved Hide resolved
@samdark samdark removed the status:code review The pull request needs review. label Jan 28, 2020
@samdark samdark merged commit b794d76 into yiisoft:master Jan 28, 2020
@samdark
Copy link
Member

samdark commented Jan 28, 2020

Merged. Thank you!

It would be cool to have it ported to Yii 3 yiisoft/arrays#15

samdark added a commit to yiisoft/arrays that referenced this pull request Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants