ArrayUtils::inArray should use strict in_array?

[adamlundrigan]

Related: zendframework/zend-form#18

In Zend\Form\View\Helper\FormSelect ArrayUtils::inArray is called with the strict parameter false. This causes an issue with string matching ('1.1' and '1.10' treated equivalent):

<?php
$needle = '1.10';
$haystack = ['1.1'];

assert(in_array($needle, $haystack) === false);
// PHP Warning:  assert(): Assertion failed in <file> on line 5

(3v4l: https://3v4l.org/HKM8Q)

Simply changing FormSelect to use strict=true breaks an existing test which uses integer keys in the value options array.

Since ArrayUtils::inArray uses string casting to work around in_array's wonky non-strict behaviour, shouldn't the call to in_array always have strict=true?

diff --git a/src/ArrayUtils.php b/src/ArrayUtils.php
index 17e3ae3..69b9721 100644
--- a/src/ArrayUtils.php
+++ b/src/ArrayUtils.php
@@ -199,7 +199,7 @@ abstract class ArrayUtils
                 }
             }
         }
-        return in_array($needle, $haystack, $strict);
+        return in_array($needle, $haystack, true);
     }

     /**

I've tested this change here (all tests pass) and against zend-form (where it fixes the reported issue).

What's the protocol for testing changes to packages which may have knock-on effects on other packages? Pull every repo that has a dependency on stdlib, apply the patch, and run the tests?

[Ocramius]

@adamlundrigan would everything be ok if you passed true to $strict?

[adamlundrigan]

@Ocramius

Simply changing FormSelect to use strict=true breaks an existing test which uses integer keys in the value options array.

I should have included a link to the source there for context. If I simply change this line to add true for $strict then I get a failure in this test case which uses integer keys for the value options because it's strictly searching for "string 0" in an array that contains "int 0".

[Maks3w]

@Ocramius inArray is not covered by tests. https://coveralls.io/builds/3895736/source?filename=src%2FArrayUtils.php#L188

[Maks3w]

I'll do the logical question. Why not

<?php
$needle = '1.10';
$haystack = ['1.1'];

assert(ArrayUtils::inArray($needle, $haystack, **true**) === false);

[Maks3w]

Sorry I wrote wrong the example. I've edited

[adamlundrigan]

Marco asked same question above: #41 (comment)

It breaks an existing test in FormSelect. The value_options array for Select can contain integer keys, which are converted to string when rendered by FormSelect. With a strict check alone that will fail since the haystack has "int 0" but the form data has "string 0". The only workaround that works for both cases is to pass "false" to ArrayUtils::inArray (so it casts everything to strings) but pass "true" to in_array to get strict matching. That's what the code in my PR does.

[Maks3w]

Sorry I'm not able to understand why ArrayUtils::inArray($needle, $haystack, **true**) fail but ArrayUtils::inArray($needle, $haystack, false) { return in_array($needle, $haystack, **true**); } works.

I've missing something in your answer.

[adamlundrigan]

ArrayUtils::inArray will cast everything to string when it's $strict argument is false:

    /**
     * Checks if a value exists in an array.
     *
     * Due to "foo" == 0 === TRUE with in_array when strict = false, an option
     * has been added to prevent this. When $strict = 0/false, the most secure
     * non-strict check is implemented. if $strict = -1, the default in_array
     * non-strict behaviour is used.
     *
     * @param mixed $needle
     * @param array $haystack
     * @param int|bool $strict
     * @return bool
     */
    public static function inArray($needle, array $haystack, $strict = false)
    {
        if (!$strict) {
            if (is_int($needle) || is_float($needle)) {
                $needle = (string) $needle;
            }
            if (is_string($needle)) {
                foreach ($haystack as &$h) {
                    if (is_int($h) || is_float($h)) {
                        $h = (string) $h;
                    }
                }
            }
        }
        return in_array($needle, $haystack, $strict);
    }

We need that, or else FormSelect view helper will blow up if you set value_options with integer keys, like this:

        $element = new SelectElement('foo');
        $element->setValueOptions([
            0 => 'label0',
            1 => 'label1',
        ]);

        // Pretend this value came from POST and we're re-displaying the form
        $element->setValue("0");

That's a valid use case. FormSelect view helper will take that and output something like this:

<!-- snip -->
<option value="0" selected="selected">label0</option>
<option value="1" selected="selected">label1</option>
<!-- / snip -->

The types don't match so ArrayUtils::inArray($n, $h, true) won't work there. ArrayUtils::inArray($n, $h, false) fixes this case by casting everything to string before chucking it into in_array.

However, PHP has another thing we need to work around:

var_dump('1.1' == '1.10');
// bool(true)

https://3v4l.org/QRkkA

Which means that ArrayUtils::inArray($n, $h, false) won't work for this case:

        $element = new SelectElement('foo');
        $element->setValueOptions([
            '1.1' => 'label 1.1',
            '1.10' => 'label 1.10',
            '1.2' => 'label 1.2',
            '1.20' => 'label 1.20',
        ]);

        // Pretend this value came from POST and we're re-displaying the form
        $element->setValue("1.1");

We'll end up with both "1.1" and "1.10" selected when we render to HTML because in PHP's bizarro world "1.1" == "1.10".

Basically, we need a check that's strict on value (so "1.1" != "1.10") but not strict type (so "int 0" == "string 0"), and that's what sending true into in_array's $strict regardless of the value provided to ArrayUtils::inArray's $strict is doing. It's making sure in_array does a strict check but casting everything to string first so types aren't an issue.

[Maks3w]

I understand the issue now.

Well, I see inArray has three responsibilities:

1. wrap in_array
2. transform needle
3. transform haystack.

My (abstract) proposal:

1. Refactor needle transformation as method.

2. Refactor haystack transformation as method.

3. Deprecate inArray in favor of :

   in_array(
     ArrayUtils::needleTransform($needle), 
     ArrayUtils::haystackTransform($haystack),
     $strict
   );

[adamlundrigan]

I think it could be distilled down to a single generic "recursively cast array keys and non-array values to string" which would cover both the needle and haystack transforms. castKeysAndValuesToString is long but leaves no ambiguity of purpose.

Regardless, it doesn't feel like the right solution. The intention of ArrayUtils::inArray was to work around the wonky behaviour of non-strict in_array, but it's not doing that job completely in it's current state.

[weierophinney]

This repository has been closed and moved to laminas/laminas-stdlib; a new issue has been opened at laminas/laminas-stdlib#5.