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

Preventing integer 0 keys from getting wrongly scrubbed #64

Closed
wants to merge 1 commit into from

Conversation

violuke
Copy link

@violuke violuke commented Jul 22, 2015

No description provided.

@brianr
Copy link
Member

brianr commented Jul 22, 2015

Thanks @violuke !

Can you tell me a little more about the behavior that this is fixing? I did some quick tests with integer keys and it seems that the regexes are getting evaluated against the integer as a string, which seems... maybe ok?

@violuke
Copy link
Author

violuke commented Jul 23, 2015

Thanks @brianr, it's actually the in_array() bit which is the problem. The key of 0 is wrongly considered as in the array of $this->scrub_fields (presumably php converts the strings in the scrub_fields to ints to compare them to the keys, rather than the other way around.

See simple test of:

$array = [
    'something',
    'something else',
    'another',
    'this' => 'this2',
    'that' => 'that2'
];

$array2 = [
    'this',
    'that'
];

foreach ($array as $key => $value){
    if (in_array($key, $array2)){
        echo $key . ' is in $array2' . "\n";
    } else {
        echo $key . ' is not in $array2' . "\n";
    }
}

Which gives:

0 is in $array2
1 is not in $array2
2 is not in $array2
this is in $array2
that is in $array2

However the key of 0 is not in the array. I hope that makes sense?

@brianr
Copy link
Member

brianr commented Jul 28, 2015

Thanks @violuke for your work here! I've merged #65 which seems to be a more robust fix for this issue. Will get a new release out with that shortly.

@brianr brianr closed this Jul 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants