Add FilterData

[franmomu]

Subject

I have doubts on where to do this, the easy way is to do it in 4.x, I thought about adding FilterDataType in 3.x, but removing buildForm from types is BC break, so not sure if is worth it to do it in 3.x.

My idea with filters was:

FilterDataType

This type will be used as a base of filter types, it contains type and value fields and it defines operator_type, field_type, operator_options and field_options options.

The idea is that other types will override getParent() method returning FilterDataType, so there is no need for them to override buildForm with type and value and only have to configure the operator_type and field_type options (and other options if they have).

The datatransform transforms from array to FilterData (I'll extract the CallbackTransformer to a class).

FilterData

This VO (ref: #6658) will be used instead of array for filters, this cannot be done with BC (at first I thought that implementing ArrayAccess could work, but array_key_exists does not work with objects and will break implementations).

I am targeting this branch, because this is not BC.

Closes #6658.

Changelog

### Added
- Added `FilterData` to model the data from a filter
### Changed
- Changed `FilterInterface::apply()` to accept a `FilterData` instance instead of `array` in argument 4

To do

• Extract CallbackTransformer to a class.
• Add tests.

[core23]

IMHO you can do this on the stable branch if you implement the ArrayAccess interface

[franmomu]

IMHO you can do this on the stable branch if you implement the ArrayAccess interface

This is what I thought at first (it's in the description) and also implemented, but we (and maybe others) are using array_key_exists like in https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/eb0f3b1ad0b1650bed6eedf732d320776f7aeccd/src/Filter/BooleanFilter.php#L40

And calling array_key_exists with an object doesn't work: https://3v4l.org/bZrdn

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

If we merge this, it will require to update the filter from persistence bundle.
I assume we'll update also all our filters (and the callback filters, so all the callback).
It will lead to a lot of code to update for a project will a lot of filters.

It would be great to find a way to provide a BC way... With an option maybe ?

[VincentLanglet]

And why this part https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Datagrid/Datagrid.php#L290-L300 is untouched ?

[phansys]

It would be great to find a way to provide a BC way... With an option maybe ?

Since these changes are targeting the next major, I think there is no need for a BC layer (which will require more work to maintain).

[VincentLanglet]

Since these changes are targeting the next major, I think there is no need for a BC layer (which will require more work to maintain).

I had in mind to target 3.x instead. For a smooth update.
If the 4.0 update requires to rewrite every existing filters, it would be too big for some projects...

[phansys]

I had in mind to target 3.x instead. For a smooth update.

In that case, I think we could follow your suggestion about the switch parameter, deprecating its usage with the value that provides the array based filters and indicating that it will be removed in the next major.

[VincentLanglet]

Maybe it could help to implement ArrayAccess in FilterData.
The only code needed to be update would be array_key_exist.

$data['value'] would still work.
And we deprecate the ArrayAccess implementation in 4.x

[franmomu]

There are already PRs to use this in the persistence bundles: sonata-project/SonataDoctrineORMAdminBundle#1407 and sonata-project/SonataDoctrineMongoDBAdminBundle#574 (to be updated to the new namespace).

I don't think it's too much work to update to use FilterData, it feels weird to implement something (ArrayAccess) in 4.x (not released) which is already deprecated. But if you think it would be useful I can implement it.

And why this part https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Datagrid/Datagrid.php#L290-L300 is untouched ?

Because it's not needed, FilterData comes from the forms, given a form (which parent is FilterDataType) when $form->getData() is called, it returns a FilterData (there is a DataTransformer).

[VincentLanglet]

I don't think it's too much work to update to use FilterData, it feels weird to implement something (ArrayAccess) in 4.x (not released) which is already deprecated. But if you think it would be useful I can implement it.

It's not a lot of for persistence bundle indeed.
But I have 50 callback filters in my work project, for instance:

    /**
     * @param ProxyQuery $query
     * @param string     $alias
     * @param string     $field
     * @param array      $data
     *
     * @return bool
     */
    public function getByTotalSavingsFilter($query, $alias, $field, $data)
    {
        if (!$data['value']) {
            return false;
        }

        $contractSupportAlias = $this->getEntityAliasForPath($query, 'contractSupports.amount');
        $parameterName = $this->generateUniqParameterName($query, 'totalSavings');
        $operator = NumberFilter::CHOICES[$data['type']] ?? '=';

        $query->andHaving("SUM($contractSupportAlias.amount) $operator :$parameterName");
        $query->groupBy($alias);
        $query->setParameter($parameterName, $data['value']);

        return true;
    }

When I'll want to update to 4.0, I'll need to update all the other BC-break (for instance the typehint) for my code.
With this PR, i'll also have to update all my filters.
As a developer I would appreciate to do the filter update later and check first that my admin is working properly with 4.0.

But maybe it's better to do something here https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Filter/CallbackFilter.php#L44
Like an option for transforming the FilterData to an array before calling the callback. (Or a way to support both ??)

It could be something introduce in 4.0 - alpha/beta and removed before the real 4.0.
This way the developper could update step by step. But maybe I'm overthinking ?

[VincentLanglet]

I'd like to release the alpha before this merge ; if it's okay for you.
It could be the step I look for.

Released

[VincentLanglet]

@franmomu @jordisala1991 If I merge this, do you prefer to

1. Release an alpha2 and then we develop the support in the persistence bundle
2. Change the minimum stability in persistence bundle to dev in order to use the dev-master branch and add compatibility before the alpha2
   ?

[jordisala1991]

I think the second option is better but not strong opinion

[franmomu]

Just realised that type could not exists, but not sure if that makes sense.

With the current implementation FilterData::getType() would return null if type is not present in the array.

[VincentLanglet]

null and no value should behave the same IMHO.

[VincentLanglet]

I might be wrong but I think this PR need to be updated since this merge 6a0c277

This #7033 should use the FilterData isn't it ?

[franmomu]

I might be wrong but I think this PR need to be updated since this merge 6a0c277

how so?

[franmomu]

just squashed the two commits and renamed methods

[VincentLanglet]

I might be wrong but I think this PR need to be updated since this merge 6a0c277

how so?

I saw here: https://github.com/sonata-project/SonataAdminBundle/pull/7033/files#diff-e279d5562ba068b14806eea28b5eea2b485848e73618a8a712dae6cc52e89e98R119 some $data['value'] and $data['type'] so I thought we had something to do...

I thought we got rid of this $data['value'], but I now understand that we only get rid of some of them

[VincentLanglet]

After this merge, we could update the persistence bundle and release an alpha-2. WDYT @sonata-project/contributors ?

[VincentLanglet]

Do you have some time to finish this @franmomu ? :)

[franmomu]

I was thinking that we can copy FilterData class in 3.x and for the CallbackFilter:

Before calling:

https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/7493cfffcbe9996316e2dd0cdd9ac5092cf62a18/src/Filter/CallbackFilter.php#L44

We can maybe check with reflection the last parameter of $this->getOption('callback').

• If the user has not defined type declaration or it is array we can trigger a deprecation that the user must add FilterData as type declaration.
• If the user already has FilterData we transform the array to FilterData.

These callables are not supposed to implement FilterInterface so I think they would be fine (not forced to not have type declaration).

The only BC break in 4.0 would be for classes implementing FilterInterface as it is now.