Added NumericRange filter which gives the possibility of filtering … #146

christopherhero · 2021-03-29T03:11:27Z

Q	A
Branch?	1.13
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	#
License	MIT

While working on adding a custom filter, I thought that it could be in the bundle.
I add range numerical filter which can filter on numerical fields in range (from-to) Like existing DateFilter.

Also in this pull request is a small change in two classes:

Doctrine\ORM\ExpressionBuilder
Doctrine\ORM\DataSource.

I suggested adding filtering capability also when using HAVING instead of WHERE in the query. For example, when we use count (*). In current architecture is hard to distinguish the type of condition (WHERE or HAVING). Another way than suggested by me is to add pubilic methods e.g havingLessThan to ORM\ExpressionBuilder and new interface "HavingExpressionBuilder"

Please give your opinion what do you think about it?

This idea also came while working on this filter because I had to filter in queries with the HAVING clause.

Roshyo · 2022-02-15T06:51:38Z

Hi @christopherhero and thank you for your PR.

But I feel this one is a bit too complex for the value it should bring.
I would also love to have a numeric filter out of the box. But IMO it should just look like the current MoneyFilter without the currency part. What do you think ?

christopherhero · 2023-06-19T08:05:22Z

@Roshyo Hi thanks :)

I have lightly changed RangeNumericalFiolter to NumericalFIlter (I also removed the logic for the expression builder, I will add that in a separate pr)

christopherhero · 2023-06-27T09:13:46Z

@Roshyo what you think now :) ?

Roshyo · 2023-06-27T09:59:10Z

LGTM. Let's wait for another approval before merging

Roshyo · 2023-06-27T10:01:43Z

@christopherhero Could you try to rebase on 1.13?

christopherhero · 2023-09-04T08:58:08Z

@Roshyo updated

src/Component/Filter/NumericalFilter.php

diimpp · 2023-09-15T15:19:19Z

+1 for adding numeric filter, current list looks lonely without it 😃

I've a naming concern, it feels more like RangeFilter, rather than NumericFilter.

How about to synchronize naming and capabilities with api-platform?
https://api-platform.com/docs/core/filters/#numeric-filter <eq>
https://api-platform.com/docs/core/filters/#range-filter <lt|gt|lte|gte|between>

christopherhero · 2023-10-30T12:05:58Z

@diimpp Thanks for review :)
Regarding Api platfrom, could you give some example of what you mean ?
Because I don't fully understand, thanks :)

christopherhero · 2024-01-08T09:46:09Z

@diimpp What else to change :) ?

diimpp

Hi @christopherhero, sorry for late reply. 🙏

Please see the comments.

Filter should allow to specify comparison strictness, like lt|gt|lte|gte, e.g. greater than and greater than or equel

diimpp · 2024-01-08T18:35:19Z

src/Bundle/Form/Type/Filter/RangeNumericFilterType.php

+use Symfony\Component\Form\FormBuilderInterface;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+
+final class RangeNumericFilterType extends AbstractType


Either

Suggested change

final class RangeNumericFilterType extends AbstractType

final class NumericRangeFilterType extends AbstractType

or

Suggested change

final class RangeNumericFilterType extends AbstractType

final class RangeFilterType extends AbstractType

@loic425 Hi, wdyt?

diimpp · 2024-01-08T18:37:59Z

src/Bundle/Form/Type/Filter/RangeNumericFilterType.php

+            ->add('greaterThan', NumberType::class, [
+                'label' => 'sylius.ui.greater_than',
+                'required' => false,
+                'scale' => $options['scale'],


Scale depends on rounding mode, so it should be possible to configure as well.

https://symfony.com/doc/current/reference/forms/types/number.html#rounding-mode

diimpp · 2024-01-08T18:38:16Z

src/Bundle/Form/Type/Filter/RangeNumericFilterType.php

+            ->add('lessThan', NumberType::class, [
+                'label' => 'sylius.ui.less_than',
+                'required' => false,
+                'scale' => $options['scale'],


diimpp · 2024-01-08T18:41:03Z

src/Bundle/Resources/config/services/filters.xml

@@ -67,6 +67,16 @@
        </service>
        <service id="sylius.form.type.grid_filter.exists" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\ExistsFilterType" />

+        <service id="Sylius\Component\Grid\Filter\RangeNumericFilter">
+            <tag name="sylius.grid_filter" type="numeric" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />


Suggested change

<tag name="sylius.grid_filter" type="numeric" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />

<tag name="sylius.grid_filter" type="numeric_range" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\NumericRangeFilterType" />

Suggested change

<tag name="sylius.grid_filter" type="numeric" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />

<tag name="sylius.grid_filter" type="range" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeFilterType" />

diimpp · 2024-01-08T18:41:24Z

src/Bundle/Form/Type/Filter/RangeNumericFilterType.php

+
+    public function getBlockPrefix(): string
+    {
+        return 'sylius_grid_filter_range_numeric';


Suggested change

return 'sylius_grid_filter_range_numeric';

return 'sylius_grid_filter_numeric_range';

Suggested change

return 'sylius_grid_filter_range_numeric';

return 'sylius_grid_filter_range';

diimpp · 2024-01-08T18:45:48Z

src/Bundle/Resources/config/services/filters.xml

+        <service id="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType">
+            <tag name="form.type" />
+        </service>
+        <service id="sylius.form.type.grid_filter.numeric" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />


Suggested change

<service id="sylius.form.type.grid_filter.numeric" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />

<service id="sylius.form.type.grid_filter.numeric_range" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\NumericRangeFilterType" />

Suggested change

<service id="sylius.form.type.grid_filter.numeric" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />

<service id="sylius.form.type.grid_filter.range" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeFilterType" />

src/Component/Filter/RangeNumericFilter.php

diimpp · 2024-01-08T19:34:05Z

src/Component/Filter/RangeNumericFilter.php

+            return;
+        }
+
+        $field = $options['field'] ?? $name;


I assume if I will write field: 1, then it would be int and we need string. Date filter do this conversion as well.

Suggested change

$field = $options['field'] ?? $name;

$field = (string) $options['field'] ?? $name;

diimpp · 2024-01-09T18:15:26Z

src/Component/Filter/RangeNumericFilter.php

+
+final class RangeNumericFilter implements FilterInterface
+{
+    public const DEFAULT_SCALE = 0;


I wonder which default scale would be sensible.

@Sylius/development-team wdyt?

SF numeric form type uses 3.

I think 0 is the most sensible. I don't think filtering with decimal precision is more common than with whole numbers.

diimpp · 2024-01-09T18:18:44Z

src/Component/Filter/RangeNumericFilter.php

+    /**
+     * @param string[] $data
+     */


Since phpstan now understands more complex docblocks, something like this should be done.

Suggested change

/**

* @param string[] $data

*/

/** @param array<array-key, string> $data */

christopherhero · 2024-01-22T08:54:34Z

@diimpp

christopherhero · 2024-02-05T09:50:28Z

what do you think about it now ;)
@diimpp @vvasiloi

src/Component/spec/Filter/NumericRangeFilterSpec.php

christopherhero · 2024-02-15T01:37:01Z

@vvasiloi

vvasiloi

Only 2 more observations I missed in the previous review, but it can be merged even without addressing those.

src/Component/Filter/NumericRangeFilter.php

…n the range (gt|gte|lt|lte) from to on numeric type fields.

NoResponseMate · 2024-02-22T11:14:49Z

Thank you, @christopherhero!

vvasiloi · 2024-02-22T11:27:56Z

Thank you @christopherhero, for having patience and finishing the work even after so much time passed! 🥇

christopherhero marked this pull request as ready for review March 29, 2021 03:12

christopherhero force-pushed the add_numerical_range_filter branch 2 times, most recently from 3b8e339 to cf854c9 Compare June 19, 2023 08:01

Roshyo requested a review from a team June 27, 2023 09:59

christopherhero force-pushed the add_numerical_range_filter branch from cf854c9 to b1f4e43 Compare August 3, 2023 23:25

christopherhero changed the base branch from master to 1.13 August 3, 2023 23:25

christopherhero force-pushed the add_numerical_range_filter branch from b1f4e43 to 89c8804 Compare August 23, 2023 13:52

Roshyo approved these changes Sep 5, 2023

View reviewed changes

vvasiloi reviewed Sep 5, 2023

View reviewed changes

src/Component/Filter/NumericalFilter.php Outdated Show resolved Hide resolved

christopherhero force-pushed the add_numerical_range_filter branch 2 times, most recently from 8595a1b to 6222fd8 Compare September 15, 2023 14:36

christopherhero force-pushed the add_numerical_range_filter branch 2 times, most recently from eb5262c to 4d7c3c8 Compare September 28, 2023 00:53

christopherhero changed the title ~~Added RangeNumerical filter which gives the possibility of filtering …~~ Added RangeNumeric filter which gives the possibility of filtering … Sep 28, 2023

christopherhero requested a review from vvasiloi September 28, 2023 00:59

diimpp reviewed Jan 9, 2024

View reviewed changes

christopherhero force-pushed the add_numerical_range_filter branch from 4d7c3c8 to 15e5aee Compare January 15, 2024 00:25

christopherhero changed the title ~~Added RangeNumeric filter which gives the possibility of filtering …~~ Added NumericRange filter which gives the possibility of filtering … Jan 15, 2024

christopherhero requested a review from diimpp January 17, 2024 09:36

vvasiloi requested changes Feb 5, 2024

View reviewed changes

src/Component/spec/Filter/NumericRangeFilterSpec.php Show resolved Hide resolved

src/Component/spec/Filter/NumericRangeFilterSpec.php Outdated Show resolved Hide resolved

src/Component/spec/Filter/NumericRangeFilterSpec.php Show resolved Hide resolved

christopherhero force-pushed the add_numerical_range_filter branch from 15e5aee to 2303bb4 Compare February 15, 2024 01:36

christopherhero requested a review from vvasiloi February 15, 2024 01:37

christopherhero force-pushed the add_numerical_range_filter branch from 2303bb4 to f7bf422 Compare February 15, 2024 03:01

vvasiloi approved these changes Feb 15, 2024

View reviewed changes

src/Component/Filter/NumericRangeFilter.php Show resolved Hide resolved

src/Component/Filter/NumericRangeFilter.php Outdated Show resolved Hide resolved

Added NumericRangeFilter which gives the possibility of filtering i…

7d669a1

…n the range (gt|gte|lt|lte) from to on numeric type fields.

christopherhero force-pushed the add_numerical_range_filter branch from f7bf422 to 7d669a1 Compare February 21, 2024 00:33

oallain approved these changes Feb 21, 2024

View reviewed changes

NoResponseMate approved these changes Feb 22, 2024

View reviewed changes

NoResponseMate merged commit e079cfb into Sylius:1.13 Feb 22, 2024
7 checks passed

NoResponseMate added the Feature New feature proposals. label Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added NumericRange filter which gives the possibility of filtering … #146

Added NumericRange filter which gives the possibility of filtering … #146

christopherhero commented Mar 29, 2021 •

edited by diimpp

Loading

Roshyo commented Feb 15, 2022

christopherhero commented Jun 19, 2023

christopherhero commented Jun 27, 2023

Roshyo commented Jun 27, 2023

Roshyo commented Jun 27, 2023

christopherhero commented Sep 4, 2023

diimpp commented Sep 15, 2023

christopherhero commented Oct 30, 2023

christopherhero commented Jan 8, 2024

diimpp left a comment

diimpp Jan 8, 2024

diimpp Jan 8, 2024

diimpp Jan 8, 2024

diimpp Jan 8, 2024

diimpp Jan 8, 2024

diimpp Jan 8, 2024

diimpp Jan 8, 2024

diimpp Jan 9, 2024

vvasiloi Feb 15, 2024

diimpp Jan 9, 2024

christopherhero commented Jan 22, 2024

christopherhero commented Feb 5, 2024

christopherhero commented Feb 15, 2024

vvasiloi left a comment

NoResponseMate commented Feb 22, 2024

vvasiloi commented Feb 22, 2024

	final class RangeNumericFilterType extends AbstractType
	final class NumericRangeFilterType extends AbstractType

	final class RangeNumericFilterType extends AbstractType
	final class RangeFilterType extends AbstractType

	<tag name="sylius.grid_filter" type="numeric" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />
	<tag name="sylius.grid_filter" type="numeric_range" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\NumericRangeFilterType" />

	<tag name="sylius.grid_filter" type="numeric" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />
	<tag name="sylius.grid_filter" type="range" form-type="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeFilterType" />

	return 'sylius_grid_filter_range_numeric';
	return 'sylius_grid_filter_numeric_range';

	return 'sylius_grid_filter_range_numeric';
	return 'sylius_grid_filter_range';

	<service id="sylius.form.type.grid_filter.numeric" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\RangeNumericFilterType" />
	<service id="sylius.form.type.grid_filter.numeric_range" alias="Sylius\Bundle\GridBundle\Form\Type\Filter\NumericRangeFilterType" />

	$field = $options['field'] ?? $name;
	$field = (string) $options['field'] ?? $name;

Added NumericRange filter which gives the possibility of filtering … #146

Added NumericRange filter which gives the possibility of filtering … #146

Conversation

christopherhero commented Mar 29, 2021 • edited by diimpp Loading

Roshyo commented Feb 15, 2022

christopherhero commented Jun 19, 2023

christopherhero commented Jun 27, 2023

Roshyo commented Jun 27, 2023

Roshyo commented Jun 27, 2023

christopherhero commented Sep 4, 2023

diimpp commented Sep 15, 2023

christopherhero commented Oct 30, 2023

christopherhero commented Jan 8, 2024

diimpp 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

christopherhero commented Jan 22, 2024

christopherhero commented Feb 5, 2024

christopherhero commented Feb 15, 2024

vvasiloi left a comment

Choose a reason for hiding this comment

NoResponseMate commented Feb 22, 2024

vvasiloi commented Feb 22, 2024

christopherhero commented Mar 29, 2021 •

edited by diimpp

Loading