Feature Request: Allow using array based $parameters with IN() expressions

[spackmat]

When I have an IN() expression, like for a EntityFilterType with 'multiple' => 'true',, I must (UPDATE: "must", see my first answer) set the params directly within the expression. Like this:

->add('myfield', EntityFilterType::class, [
    // ...
    'class' => MyIntKeyedEntity::class, // this one has an integer primary key
    'multiple' => true,
    'apply_filter' => function (QueryInterface $filterQuery, string $field, array $values): ?ConditionInterface {
        // ... 
        return $filterQuery->createCondition(
            $filterQuery->getExpr()->in($field, array_map(fn(MyIntKeyedEntity $value) => $value->getId(), $values['value']->toArray()))
        );
    },
])

This workaround was just a bit ugly (and quite surprising), but when it comes to UUIDs as primary keys with ORM, it just does not work, because those must be in binary format to work correctly. So what I would do instead is something like this:

->add('myfield', EntityFilterType::class, [
    // ...
    'class' => MyUuidKeyedEntity::class, // this one has a Uuid primary key
    'multiple' => true,
    'apply_filter' => function (QueryInterface $filterQuery, string $field, array $values): ?ConditionInterface {
        // ...
        return $filterQuery->createCondition(
            $filterQuery->getExpr()->in($field, ':p_myparam'),
            ['p_myparam' => array_map(fn(MyUuidKeyedEntity $value) => $value->getId()->toBinary(), $values['value']->toArray())]
        );
    },
])

But that also does not work and after some investigation to insanity, I found the cause of this and it is the following snippet in the DoctrineApplyFilterListener currently on line 52:

foreach ($this->parameters as $name => $value) {
    if (is_array($value)) {
        list($value, $type) = $value;
        $qbAdapter->setParameter($name, $value, $type);
    } else {
        $qbAdapter->setParameter($name, $value);
    }
}

That list() assumes that if an array is given as a parameter's value, it wants to set a [value, type] pair. But for IN() expressions (or maybe other expressions expecting a list of values), an array is just a list of values like [value1, value2, value3]. So in a first approach, I secure that assumption with two more checks:

foreach ($this->parameters as $name => $value) {
    if (is_array($value) && count($value) === 2 && \Doctrine\DBAL\Types\Type::hasType($value[1])) {
        list($value, $type) = $value;
        $qbAdapter->setParameter($name, $value, $type);
    } else {
        $qbAdapter->setParameter($name, $value);
    }
}

With that simple modification, my IN() expression works fine with its values given as a proper parameter and that somewhat strange type-enabling-array-thing still works. Maybe that should be removed anyways, as from my point of view this is not a good solution anyhow. Maybe for that usecase (providing a type for a value) a special object could be implemented holding the value/values-array and the optional type. That would be a BC break, but would be a less hacky solution to allow both the type giving and value-arrays. For now, my solution does work without a BC break, but with the possibility to break with edgecases.

@spike31 Would you accept a PR for that? Or do you have a better idea to allow UUIDs in such cases?

[spackmat]

I found another workaround: Instead of setting an array as the parameter value, I can keep as/convert to an ArrayCollection, so that is_array() branch in DoctrineApplyFilterListener does not apply at all. I don't know if that works in all cases and it is not how Doctrine handles parameters on in() expressions, but in my case that works well:

->add('myfield', EntityFilterType::class, [
    // ...
    'class' => MyEntity::class,
    'multiple' => true,
    'apply_filter' => function (QueryInterface $filterQuery, string $field, array $values): ?ConditionInterface {
        // ...
        // $values['value'] already is an ArrayCollection in that context (EntityFilterType with multiple option set to true)
        return $filterQuery->createCondition(
            $filterQuery->getExpr()->in($field, ':p_myparam'),
            ['p_myparam' => $values['value']->map(fn(MyEntity $value) => $value->getId()->toBinary())]
        );
    },
])

Nevertheless: I'll provide a PR on that topic soon that makes that part of the code more clean and introduces a ExpressionParameterValue class that holds a parameter value for an expression and optionally its type in a clean way. So that that old and ambiguous handling of array-values can be deprecated in favor of that ExpressionParameterValue value object.

[spackmat]

This is closely related to #362 in a way that it only came up for me, because that default implementation of the EntityFilterType cannot handle non-integer primary keys. So that also could be fixed for those standard cases of the EntityFilterType without an additional 'none' option that at the moment must be handled by a ChoiceFilterType anyway.