letsdrink / ouzo

Ouzo Framework - PHP MVC ORM
https://github.com/letsdrink/ouzo
MIT License
70 stars 8 forks source link

Fix issue in Arrays::getNestedValue #278

Closed IlyaPokamestov closed 5 years ago

IlyaPokamestov commented 5 years ago

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

IlyaPokamestov commented 5 years ago

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

danon commented 5 years ago

Hey, thanks for the PR :)

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

Two questions:

IlyaPokamestov commented 5 years ago

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 commented 5 years ago

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

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 commented 5 years ago

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

IlyaPokamestov commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

IlyaPokamestov commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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.