Add Array filter to search value in @ORM\Column(type="array")

[VincentLanglet]

Subject

This filter allow to search for specific value when you use @ORM\Column(type="array").

Let's say you have in database
1 => ['a']
2 => ['b']
3 => ['c']
4 => ['a', 'b']
5 => ['a', 'c']
6 => ['a', 'b', 'c']

If you search

• CONTAINS ['a'] ; you'll get 1,4,5,6.
• NOT CONTAINS ['a'] ; you'll get 2,3.
• EQUAL ['a'] ; you'll get 1.
• CONTAINS ['a', 'b'] ; you'll get 4,6.
• NOT CONTAINS ['a', 'b'] ; you'll get 3.
• EQUAL ['a', 'b'] ; you'll get 4.

I am targeting this branch, because BC.

Closes #1029

Changelog

### Added
- `ArrayFilter` which supports `@ORM\Column(type="array")`

[core23]

If I got you wrong, you have to input the string ['a'] to get any result. This is very technical for a normal user.

[franmomu]

IMHO we should not support filter for a ORM\Column(type="array") field, this kind of field (serialize) is not made for queries in SQL, it is quite complex and users can get confused.

IIUC the implementation, it only works with plain arrays of strings, but this field could contain basically anything.

If someone wants to query a field like that, IMHO should consider using another type field or mecanism.

[VincentLanglet]

If I got you wrong, you have to input the string ['a'] to get any result. This is very technical for a normal user.

No it works like the ChoiceFilter, you can provide an array or a single element, since there is the following code:

if (!\is_array($data['value'])) {
    $data['value'] = [$data['value']];
}

You can use it

->add('foo', ArrayFilter)

Or

->add('foo', ArrayFilter, [], ChoiceType, [ ... ])

IIUC the implementation, it only works with plain arrays of strings, but this field could contain basically anything.

This dont works with strings only. For example if you look for the value true, it will look for b:1;.

IMHO we should not support filter for a ORM\Column(type="array") field, this kind of field (serialize) is not made for queries in SQL
If someone wants to query a field like that, IMHO should consider using another type field or mecanism.

The ORM\Column(type="array") is may not the best to make query but if someone use it, he has his reasons. Developers doesn't always have the hand over the Database.
IMHO we should support every ORM\Column type if possible.

This PR close an opened issue, which was approved by others, and we have a working solution to provide.

it is quite complex and users can get confused.

This only means that we need to document it here https://sonata-project.org/bundles/doctrine-orm-admin/master/doc/reference/filter_field_definition.html.

I'll add documentation.

[greg0ire]

Please do atomic commits, right now you have one commit for the new feature, one commit for its documentation, and that second commit contains unrelated things, and then there is one more commit about improving documentation that does not say why there is an improvement or even what that improvement is. git reset origin/3.x && git add --patch should help with this.

[VincentLanglet]

Should be better now @greg0ire

[greg0ire]

Gave this a technical review, but maybe concerns raised by @franmomu are justified?

Developers doesn't always have the hand over the Database.

Surely if you develop such an application you can request a column to be added or things like that, can't you?

The ORM\Column(type="array") is may not the best to make query but if someone use it, he has his reasons.

I think that happened to me in the distant past, and I think the reason was I did not have the filtering requirement from the beginning, so I ended up either using LIKE as you did or, when I had the time and motivation, move to a separate enum-like entity.

It does work but I didn't contribute it because it has a few implications, like what if the user saves an associative array? What if the user uses values or keys that are literally s:? That last one make me switch to json_array. Maybe there should be some restrictions, to avoid generating support.

There are also performance implications, this will not perform well on big tables because of the LIKE % makes indices unusable.

What I'm trying to say is that giving power to developers, "treating them like adults" like some people like to say, can lead to weird bugs/performance issues that can generate a lot of support. I'm a bit undecided on this feature, but one thing I am sure of is that this shouldn't be merged without at least big fat warnings in the documentation, and requires maybe even some restrictions in the code.

[greg0ire]

Suggested change

	``Sonata\AdminBundle\Form\Type\ChoiceType`` supports filtering on value saved in database as a serialized array with ``@ORM\Column(type="array")`` annotation.
	``Sonata\AdminBundle\Form\Type\ChoiceType`` supports filtering on values saved in databases as serialized arrays with the ``@ORM\Column(type="array")`` annotation.

[greg0ire]

Please comply with the sf docs standards:

Each line should break approximately after the first word that crosses the 72nd character (so most lines end up being 72-78 characters);

[greg0ire]

Suggested change

	protected function configureDatagridFilters(DatagridMapper $datagridMapper)
	protected function configureDatagridFilters(DatagridMapper $datagridMapper): void

[greg0ire]

Suggested change

	public function filter(ProxyQueryInterface $queryBuilder, $alias, $field, $data)
	public function filter(ProxyQueryInterface $queryBuilder, $alias, $field, $data): void

[greg0ire]

Suggested change

	public function testEmpty(): void
	public function testItStaysDisabledWhenFilteringWithAnEmptyValue(): void

[greg0ire]

Suggested change

	public function testNullValue(): void
	public function testFilteringWithNullReturnsArraysThatContainNull(): void

[greg0ire]

Consider using a data provider with

{
    yield 'explicit contains' => [ContainsOperatorType::TYPE_CONTAINS];
    yield 'implicit contains' => [null];
}

[greg0ire]

Is there a reason not to do this assertion the first time?

[greg0ire]

There should be more tests with multiple values IMO, because the behavior is debatable: should it return arrays that contain all the values or any of them? That should be clearly expressed in the test.

[greg0ire]

Should be better now @greg0ire

It's a bit better indeed, for instance the 3rd commit is ok, apart from one thing; it should be first, and all the other commits should be squashed together, because it makes no sense to separate them:

• reverting the code without reverting the tests makes no sense, and vice versa, and same with docs
• every piece of code is supposed to come with tests and docs, it's only a separate type of things, but it is part of your logical change.

[VincentLanglet]

Gave this a technical review, but maybe concerns raised by @franmomu are justified?

Developers doesn't always have the hand over the Database.

Surely if you develop such an application you can request a column to be added or things like that, can't you?

The ORM\Column(type="array") is may not the best to make query but if someone use it, he has his reasons.

I think that happened to me in the distant past, and I think the reason was I did not have the filtering requirement from the past, so I ended up either using LIKE as you did or, when I had the time and motivation, move to some a separate enum like entity.

It's indeed easily possible to add new column, but not always easy to migrate some existing columns when the ORM\Column(type="array") was already used. Not everybody can find time and motivation to do the migration. It's harder if you're not the one managing the database.

It does work but I didn't contribute it because it has a few implications, like what if the user saves an associative array? What if the user uses values or keys that are literally s:? Maybe there should be some restrictions, to avoid generating support.

I tried a general approach without too much context in the filter and be as far as possible usable and re-usable. But I'm ok with the fact to add restrictions. How would you add them ?

I can also change the name of the filter, if it helps. Since the most useful cases are when you use it with an array of string, I change to something like ArrayStringFilter

There are also performance implications, this will not perform well on big tables because of the LIKE % makes indices unusable.

What I'm trying to say is that giving power to developers, "treating them like adults" like some people like to say, can lead to weird bugs/performance issues that can generate a lot of support.

I was more like "This is already useful for me, this will certainly be useful for others, let's share it. Plus thanks to open-source and contribution, this will be permanently improved other time.". But I understand your point.

I'm a bit undecided on this feature, but one thing I am sure of is that this shouldn't be merged without at least big fat warnings in the documentation, and requires maybe even some restrictions in the code.

I'm open to any "big fat warnings" or "restrictions" to add. @greg0ire ;)

[greg0ire]

How would you add them ?

I would maybe disallow searching for characters used for serialization, like : or "

ArrayStringFilter

StringListFilter filter? See https://twitter.com/Ocramius/status/1252560762238894080

I'm open to any "big fat warnings" or "restrictions" to add. @greg0ire ;)

Then please do. Some can be forbidden in the code directly, like the above, some others should be warned about: "You should use this if you can't have that list in a separate table, and the list is not used with a table that is too big, and your list consist only in simple strings that don't contain metacharacters like " or :. Even then be wary that this will give confusing results if your lists consists in elements like s, b,…". Also, you should maybe warn about associative array (I don't know if it's possible to save ["foo" => "bar"] with this ORM type, but from the docs alone it seems possible).

[VincentLanglet]

How would you add them ?

I would maybe disallow searching for characters used for serialization, like : or "

: is search in the array as s:1:":";, so there should be no problems, same for ".
I'm not sure there is issue with these values.

ArrayStringFilter

StringListFilter filter? See https://twitter.com/Ocramius/status/1252560762238894080

I do like StringListFilter.

Even then be wary that this will give confusing results if your lists consists in elements like s, b,…".

Since I search for the serialize value s:1:"b";, this should work correctly.

Also, you should maybe warn about associative array (I don't know if it's possible to save ["foo" => "bar"] with this ORM type, but from the docs alone it seems possible).

Yes I need to add a warning indeed.

[franmomu]

You can save any serializable thing:

<?php

$object = new \StdClass();
$object->a = [
    's' => 's:1:"s"'
];
$object->s = 's:1:"s"';

$data = [
    $object,
    ['s' => 's:1:"s"'],
    ['s' => [
        's' => 's'
    ]]
];

resulting in:

a:3:{i:0;O:8:"stdClass":2:{s:1:"a";a:1:{s:1:"s";s:7:"s:1:"s"";}s:1:"s";s:7:"s:1:"s"";}i:1;a:1:{s:1:"s";s:7:"s:1:"s"";}i:2;a:1:{s:1:"s";a:1:{s:1:"s";s:1:"s";}}}

That's why I said it could be complex. What I meant is what @greg0ire said, doing this could lead to more issues (as it is not a full solution). In the case of a list of string I guess it makes more sense to use simple_array type.

[VincentLanglet]

In the case of a list of string I guess it makes more sense to use simple_array type.

array is still more robust than simple_array for list of string, since you cannot use , in your values saved in the array to avoid conflict with the implode/explode.

[VincentLanglet]

@greg0ire Is this filter now technically OK for you ?

@sonata-project/contributors Can we discuss about the pro/con about adding this filter ?