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 issue in Arrays::getNestedValue #278

Merged
merged 2 commits into from Oct 1, 2019

Conversation

@DarioSwain
Copy link
Contributor

commented Sep 27, 2019

Fix Arrays::getNestedValue in case when keys lead to non-existing element with scalar parent.

@DarioSwain

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

Hi @woru @Danon, could you please take a look on this PR? Thanks!

@Danon

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

Hey, thanks for the PR :)

Could you please illustrate what was the outcome of getNestedValueShouldReturnNullWhenZeroKeyNotFound() before your fix?

Two questions:

  • Is '0' a special value, or will the test also fail for '1'?
  • Could you remove var_dump() from test?
…ment with scalar parent.
@DarioSwain DarioSwain force-pushed the DarioSwain:master branch from e8384f8 to 07ade91 Sep 27, 2019
@DarioSwain

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

Hi @Danon,
Sorry, missed this var_dump 😄

Before this fix getNestedValue returned: "value".
Due to self::toArray($array) which is wrapping any scalar value into array, and answer for you first question is: it's only for '0', because self::toArray('value') => ['value'] and in this case 0 key is exists, but all other keys will returns null on self::getValue.

I've updated PR. Thank you for quick response!

@Danon

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

I suppose a simmilar behaviour could be recreated with these lines?

  • Arrays::getNestedValue($array, ['1', '2', '3', 0]);
  • Arrays::getNestedValue($array, ['1', '2', '3', '0', '0']);

Maybe you could try those, and if they do recreate the bug, see if your fix also fixes them?

Ps: how do you like using ozuo so far? ;)

@Danon

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

Also, @DarioSwain please make sure the bug doesn't occur in Arrays::hasNestedKey() and Arrays::removeNestedKey().

@DarioSwain

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

@Danon yes, for both your examples will be returned "value" instead of null. My fix is also tackle them, I've added few tests to cover those cases.

Regarding hasNestedKey() and removeNestedKey(), you're right, them also do not handle non-existing keys properly, I've added few checks in my latest commit (+ tests).

Ps: how do you like using ozuo so far? ;)

I'm just experimenting with ouzo-goodies, can't say anything regarding framework itself.

@Danon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Thanks a lot!!

@Danon yes, for both your examples will be returned "value" instead of null. My fix is also tackle them, I've added few tests to cover those cases.

Are you sure all your new tests fail, if your fix is not applied? :) Just checking.

Regarding hasNestedKey() and removeNestedKey(), you're right, them also do not handle non-existing keys properly, I've added few checks in my latest commit (+ tests).

Ok, thanks :)

One additional, case in my mind. What's the resulting array from

Arrays::setNestedValue($array, ['1', '2', '3', 0], "new value");

Can you also check this method for me? That's the last one, I promise :)

@Danon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Ps: how do you like using ozuo so far? ;)

I'm just experimenting with ouzo-goodies, can't say anything regarding framework itself.

Ok, how do you like Ouzo Goodies? :D

@DarioSwain

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

Are you sure all your new tests fail, if your fix is not applied? :) Just checking.

Yes.
image

Arrays::setNestedValue($array, ['1', '2', '3', 0], "new value");

This case is working fine, it returns: ['1' => ['2' => ['3' => [ 0 => 'new value']]]]

Oku, how do you like Ouzo Goodies? :D

I like it, a lot of useful tools which you haven't to redevelop from scratch, great work 👍

@Danon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Are you sure all your new tests fail, if your fix is not applied? :) Just checking.

Yes.

Ok, thank you for that double check :)

Arrays::setNestedValue($array, ['1', '2', '3', 0], "new value");

This case is working fine, it returns: ['1' => ['2' => ['3' => [ 0 => 'new value']]]]

Great we got that out of our way :)

Ok, now we only need to ask @bbankowski @piotrooo @andrzejo for a review. We'll probably have to wait through the weekend.

@bbankowski bbankowski merged commit 79a741a into letsdrink:master Oct 1, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbankowski

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@DarioSwain kudos for the fix, it looks good! I've just merged it into master. It will go with the next release, which we will hopefully do very soon :)

@Danon thanks for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.