Operator filter #83

CheapHasz · 2018-12-06T16:15:43Z

Here is a first implementation to address #64.

Is there a place I should add type shortcuts ? Something like 'alterphp_gte_text'.

I used a TextType, but maybe a NumberType would be more useful.

I was also thinking of experimenting with DateTypes , which would be a obvious candidate for operator filters.

src/EventListener/PostQueryBuilderSubscriber.php

alterphp · 2018-12-07T09:36:07Z

Hi @CheapHasz ! This looks nice !

My suggestions to improve implementation and extensibility :

Use a manager in order to list filters form operators,
Use a compiler pass to list all operator form types (and inject the list to their manager)
Make ListFormFiltersConfigPass::configureFilter method to check and replace the form alias in the case $filterConfig['type'] is defined (use the manager to replace aliases),
Using a transformer is a good idea but I think they should transform the data returned from submitted form so that you don't have to deal with transformers in PostQueryBuilderSubscriber => Operator form type could return a specific object (OperatorModel ?) that holds operator (the constant) and the value. PostQueryBuilderSubscriber test something like case $value instanceof OperatorModel.
Do we need a concrete Transformer class for each operator ? What if the constant OPERATOR_PREFIX is directly held by the form type class ?

Hoping i'm clear enough. Please let me know if you need help and/or more details !

Anyway thanks for this first implementation, this seems to be a good base !

coveralls · 2018-12-08T08:25:40Z

Coverage decreased (-4.0%) to 86.95% when pulling bff5129 on CheapHasz:operator-filter into 765dfb3 on alterphp:refactor/list-filters.

CheapHasz · 2018-12-10T10:19:17Z

Hi @alterphp ,
I won't be able to work on this issue until wednesday, but I'll try to clarify the points before I can get on it.

Use a manager in order to list filters form operators,
Use a compiler pass to list all operator form types (and inject the list to their manager)

I didn't get what you mean by that

Make ListFormFiltersConfigPass::configureFilter method to check and replace the form alias in the case $filterConfig['type'] is defined (use the manager to replace aliases),

That part is only about replacing aliases ? wouldn't adding them to the AlterPHP\EasyAdminExtensionBundle\Configuration\ShortFormTypeConfigPass make more sense ?

Using a transformer is a good idea but I think they should transform the data returned from submitted form so that you don't have to deal with transformers in PostQueryBuilderSubscriber => Operator form type could return a specific object (OperatorModel ?) that holds operator (the constant) and the value. PostQueryBuilderSubscriber test something like case $value instanceof OperatorModel.

It might be a good idea indeed, I'll think about it.

Do we need a concrete Transformer class for each operator ? What if the constant OPERATOR_PREFIX is directly held by the form type class ?

This idea stems from the fact that multiple FormType might be used for GreaterThan (off the top of my head, Number, Date and DateTime). But we still could factor the logic in one place, and the transformer seemed a good place. It also allows an external to create a new GreaterThan*Type and keep all the logic by simply adding the transformer.
Setting the constant in each of them would have messed with the tests in the PostQueryBuilderSubscriber.

Now if the logic is indeed refactored using an OperatorModel, that logic might indeed be obsolete .

(if some points are easier to explain in French, fell free to pm me directly using it).

alterphp · 2018-12-11T08:42:29Z

Hi @CheapHasz !

No worries for not having time to work on it every day, we all have some work and fortunately time doing anything but coding !

While thinking about the feature, I'm not sure handling the operator into the form type is the best implementation. Maybe the good move is to apply operator as a modifier of the form type => Transformer seems matching our needs.

What about a single ListFilterType that adds a form type (text, number, date) based on a input_type form option and its own options with input_type_option with TRUE inherit_data option, handles the operator as an optional operator form option (default is equals unless input is a multiple choice -> in operator), and returns a ListFilter ~~OperatorModel~~ that holds value and operator.

Operator list :

equals
in
gte
gt
lte
lt

Every list form filters (even equals) would be handled through the same process (with ListFilter operator/value object).

Forget about the manager and compiler pass (the idea was to allow custom filter), this is over-engineered for now.

What do you think ? I'm working on upgrade for EasyAdmin 2.0 at the time, I'll be able to help on operators after that. And I think those operators will be available for 2.x brnach only if there is an important BC break...

CheapHasz · 2018-12-12T11:43:35Z

Hi @alterphp .

New implementation, not quite sure if it was what you had in mind, but it probably is better than what I did previously.

To prevent BC breaks, I didn't refactor everything using the ListFilter, and instead use it more as an OperatorModel.
I added a 'property' attribute, to allow multiple ListFilterType on the same field (ie a min and a max)
and 'input_type' / 'input_type_options' type_option, to allow configuration of the unerlying input.

Tell me what you think

src/Form/Type/ListFilterType.php

src/Configuration/ShortFormTypeConfigPass.php

alterphp · 2018-12-12T14:31:53Z

Thanks for this update, this seems simpler.

I did not expect the property/field (when you have many filters on the same property), I'm not willing to delegate the responsibility to ListFilter objects. I'm thinking about dealing with it straight in list.form_filters config...

alterphp · 2018-12-12T14:35:03Z

I have changed the target branch so that I could merge and continue working on it on my side.

…erType

alterphp

Hi @CheapHasz, and thank you for restarting this PR ! I've mostly been working on new EasyAdmin last weeks but I'm willing to advance on your PR.

Do you need the support of EasyAdmin 1.x on the ListFilter feature ?

alterphp · 2019-01-21T21:55:10Z

src/Form/Type/ListFilterType.php

@@ -32,7 +32,7 @@ public function buildForm(FormBuilderInterface $builder, array $options)
            ->add('operator', HiddenType::class, [
                'data' => self::$operators[$options['operator']]
            ]);
-        if ($options['property'] !== null) {
+        if (isset($options['property'])) {


If property is not intended to be modified on client side, it's not required to pass it to the view form. WDYT about setting the value on ListFilter object data on POST_SET_DATA FormEvent ?

I agree with the logic, and think it could be applied to the operator property

alterphp · 2019-01-21T21:57:39Z

src/Form/Type/ListFilterType.php

+                'required' => false
+            ])
+            ->add('operator', HiddenType::class, [
+                'data' => self::$operators[$options['operator']]


I think it's better to use static::$operators for a inheritance.

By the way, you use static at line 56.

alterphp · 2019-01-21T22:06:12Z

src/Form/Type/ListFilterType.php

+
+class ListFilterType extends AbstractType
+{
+    public static $operators = [


$operators do not need to hold ORM version here, only the keys. I think this is QueryBuilder job to translate equals into = and so on.

We should use constant to define the string for operators like :

const OPERATOR_EQUALS = 'equals'; const OPERATOR_GT = 'gt'; const OPERATOR_GTE = 'gte'; const OPERATOR_LT = 'lt'; const OPERATOR_LTE = 'lte'; private static $operators = [ static::OPERATOR_EQUALS, static::OPERATOR_GT, static::OPERATOR_GTE, static::OPERATOR_LT, static::OPERATOR_LTE, ];

I'm all for using constants.

The problem I see with delegating the translation to symbol in the QueryBuilder is that it couples the PostQueryBuilderSubscriber and ListFilterType, with no simple way of implementing any new operator, modifying existing ones.

Not that we need that, but my implementation allowed to handle all that logic in the FilterType

There is a reason behind this willing : I developed a port of EasyAdmin for MongoDB documents, and EasyAdminExtension has a branch that is compatible with both entities and documents.

Operators like = or >= have no sense for Mongo query builder... That's why I prefer delegating DB query building to the builder.

I know this is more work to add filters, but decoupling the form from the DB operators makes sense.

FYI https://github.com/alterphp/EasyAdminExtensionBundle/tree/feature/mongo-odm-compat/src/EventListener

Ok it makes sense

alterphp · 2019-01-21T22:13:36Z

src/Form/Type/ListFilterType.php

+    public function configureOptions(OptionsResolver $resolver)
+    {
+        $resolver->setDefaults(array(
+            'operator' => 'equals',


Better not use a raw value here. Constant as suggested earlier ?

alterphp · 2019-01-21T22:15:20Z

src/EventListener/PostQueryBuilderSubscriber.php

+                    break;
+                }
+
+                $filterDqlPart = $field.' ' .$value->getOperator() .' :'.$parameter;


IMO, QueryBuilder is responsible of selecting the ORM operator matching data returned by ListFilter::getOperator method

alterphp · 2019-01-21T22:28:56Z

src/EventListener/PostQueryBuilderSubscriber.php

+            $property = $field;
+            if ($value instanceof ListFilter && $value->getProperty()) {
+                // if a property is specified in the ListFilter, it is on that property that we must filter
+                $property = $queryBuilder->getRootAlias().'.' .$value->getProperty();


Prefixing with root alias is not only for ListFilter. It has to be done just after this if statement.

which is what happens at lign 128, no ?

Maybe my mistake is not doing that test before the $field assignation though

Right.

On line 132, prefixing by root alias must only be done when there is no dot in the property returned by $value->getProperty(). This allows to filter on a relation property.

alterphp · 2019-01-21T22:29:43Z

src/EventListener/PostQueryBuilderSubscriber.php

            // Add root entity alias if none provided
            $field = false === \strpos($field, '.') ? $queryBuilder->getRootAlias().'.'.$field : $field;
+            $property = $field;


I prefer $ormField instead of $property.

alterphp · 2019-01-24T14:53:42Z

Sorry, my mistake

alterphp · 2019-01-24T16:50:34Z

@CheapHasz I'm taking care of requested changes. I will open another PR to compare.

alterphp · 2019-01-24T16:58:59Z

Closing in favor of #92

Jean-Pascal Devierne added 4 commits December 6, 2018 16:19

Added operator handling in PostQueryBuilderSubscriber

529b0ea

Added GreaterThanFilterTextType as a POC

d00f5f5

Added GTE, LT, LTE filter text types

1bf2607

PHP Cs Fixer

6951d94

alterphp reviewed Dec 7, 2018

View reviewed changes

src/EventListener/PostQueryBuilderSubscriber.php Outdated Show resolved Hide resolved

Jean-Pascal Devierne added 4 commits December 12, 2018 12:37

Removed all operators

70fe155

Added ListFilter and ListFilterType

25d7ca5

Added operator as a ShortFormType for ListFilterType

f4f4905

Added ListFilter handling in PostQueryBuilderSubscriber

c3c788a

alterphp requested changes Dec 12, 2018

View reviewed changes

src/Form/Type/ListFilterType.php Outdated Show resolved Hide resolved

src/Form/Type/ListFilterType.php Outdated Show resolved Hide resolved

src/Configuration/ShortFormTypeConfigPass.php Outdated Show resolved Hide resolved

alterphp changed the base branch from master to refactor/list-filters December 12, 2018 14:34

Jean-Pascal Devierne added 6 commits January 17, 2019 11:47

Use setAllowedValues to check for valid operator values

3d74f1f

Clean unnecessary variable unsetting

8af56cc

Replace operator by list_filter as $nativeShortFormTypes for ListFilt…

2a5d537

…erType

Fix allowed values to correctly restrict on keys

02d0be2

fix $options['property'] in case it is not set

3ce6141

correctly use yoda conditions

bff5129

alterphp requested changes Jan 21, 2019

View reviewed changes

alterphp closed this Jan 24, 2019

alterphp changed the base branch from refactor/list-filters to master January 24, 2019 14:53

alterphp reopened this Jan 24, 2019

alterphp mentioned this pull request Jan 24, 2019

Refactor list filters #92

Merged

alterphp closed this Jan 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator filter #83

Operator filter #83

CheapHasz commented Dec 6, 2018 •

edited

alterphp commented Dec 7, 2018

coveralls commented Dec 8, 2018 •

edited

CheapHasz commented Dec 10, 2018

alterphp commented Dec 11, 2018

CheapHasz commented Dec 12, 2018 •

edited

alterphp commented Dec 12, 2018

alterphp commented Dec 12, 2018

alterphp left a comment

alterphp Jan 21, 2019

CheapHasz Jan 24, 2019

alterphp Jan 21, 2019

alterphp Jan 21, 2019

CheapHasz Jan 24, 2019

alterphp Jan 21, 2019

alterphp Jan 21, 2019

CheapHasz Jan 24, 2019

alterphp Jan 24, 2019

alterphp Jan 24, 2019

CheapHasz Jan 24, 2019

alterphp Jan 21, 2019

CheapHasz Jan 24, 2019

alterphp Jan 21, 2019

alterphp Jan 21, 2019

CheapHasz Jan 24, 2019

alterphp Jan 24, 2019

alterphp Jan 21, 2019

CheapHasz Jan 24, 2019

alterphp commented Jan 24, 2019

alterphp commented Jan 24, 2019

alterphp commented Jan 24, 2019

Operator filter #83

Operator filter #83

Conversation

CheapHasz commented Dec 6, 2018 • edited

alterphp commented Dec 7, 2018

coveralls commented Dec 8, 2018 • edited

CheapHasz commented Dec 10, 2018

alterphp commented Dec 11, 2018

CheapHasz commented Dec 12, 2018 • edited

alterphp commented Dec 12, 2018

alterphp commented Dec 12, 2018

alterphp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alterphp commented Jan 24, 2019

alterphp commented Jan 24, 2019

alterphp commented Jan 24, 2019

CheapHasz commented Dec 6, 2018 •

edited

coveralls commented Dec 8, 2018 •

edited

CheapHasz commented Dec 12, 2018 •

edited