Delegate filter query by empty value to filters #6402

peter-gribanov · 2020-09-22T10:08:20Z

Subject

If the filter value is empty, but the filtering type has been changed from the default value, then this is an indication that the user wants to search by empty value.
At this level, we cannot determine whether a search can be performed on an empty value and therefore delegate it to filters.

Part of fix for #3569.

Changelog

### Changed
- Delegate filter query by empty value to filters.

VincentLanglet · 2020-09-22T10:24:50Z

If the filter value is empty, but the filtering type has been changed from the default value, then this is an indication that the user wants to search by empty value.

I understand your point, that's not a bad idea.
I'm just afraid that some users could by tricked by this, if they were erasing the filter value to disable the filter.
It's even more tricky since we can hide the filtering type AFTER having modified it.

ping @sonata-project/contributors

src/Datagrid/Datagrid.php

peter-gribanov · 2020-09-23T10:00:06Z

src/Datagrid/Datagrid.php

+    {
+        if (isset($this->values['_per_page'])) {
+            if (isset($this->values['_per_page']['value'])) {
+                return (int) $this->values['_per_page']['value'];


I assume that this data comes from a request and it contains a string with a number, not a integer. Therefore, i am doing explicit type casting.

It's never too safe

SonataCI · 2020-10-30T13:34:03Z

Could you please rebase your PR and fix merge conflicts?

VincentLanglet · 2020-11-04T14:21:35Z

An upgrade note seems nice to explain the (big) impact of this change.
If the user doesn't check for empty value in his filter, it will be applied.

peter-gribanov · 2020-11-09T11:30:27Z

UPGRADE-3.x.md

+### Empty values in datagrid filters
+
+Now empty values are passed to datagrid filters. If you have custom datagrid filters, add empty string check to them.
+


@VincentLanglet please review

Adding an exemple could be helpful:

Something like

In the following exemple

->add('with_open_comments', CallbackFilter::class, [ 'callback' => static function(ProxyQueryInterface $queryBuilder, string $alias, string $field, array $value): bool { if (!$value['value']) { return false; } $queryBuilder ->leftJoin(sprintf('%s.comments', $alias), 'c') ->andWhere('c.status = :status') ->setParameter('status', Comment::STATUS_MODERATE); return true; }, 'field_type' => CheckboxType::class ]);

The !$value['value'] check is required to not filtering by '' if you didn't used the filter.

VincentLanglet · 2020-11-09T13:10:39Z

You requested changes, can you take a new look @core23 ? =)

core23 · 2020-11-10T10:02:00Z

src/Datagrid/Datagrid.php

+
+    private function applySorting(): void
+    {
+        if (isset($this->values['_sort_by'])) {


Can you invert this check to fail fast

core23 · 2020-11-10T10:02:09Z

src/Datagrid/Datagrid.php

+
+    private function getMaxPerPage(int $default): int
+    {
+        if (isset($this->values['_per_page'])) {


core23 · 2020-11-10T10:02:17Z

src/Datagrid/Datagrid.php

+
+    private function getPage(int $default): int
+    {
+        if (isset($this->values['_page'])) {


UPGRADE-3.x.md

extract code from Datagrid::buildPager() into separate methods to improve readability add upgrade info add example invert check to fail fast fix text in UPGRADE

VincentLanglet · 2020-11-10T19:24:53Z

Thanks ! @peter-gribanov

This was referenced Sep 22, 2020

Impossible filter a list by empty value #3569

Closed

Allow empty string for filtering sonata-project/SonataDoctrineORMAdminBundle#1120

Merged

core23 requested changes Sep 22, 2020

View reviewed changes

src/Datagrid/Datagrid.php Outdated Show resolved Hide resolved

peter-gribanov force-pushed the filter_by_empty_value branch 3 times, most recently from 1e38aa1 to 20e6b90 Compare September 23, 2020 09:55

VincentLanglet previously approved these changes Sep 23, 2020

View reviewed changes

peter-gribanov commented Sep 23, 2020

View reviewed changes

peter-gribanov requested a review from core23 September 23, 2020 10:22

VincentLanglet requested a review from a team September 27, 2020 15:33

SonataCI added the pending author label Oct 30, 2020

peter-gribanov dismissed VincentLanglet’s stale review via d783be4 November 4, 2020 11:49

peter-gribanov force-pushed the filter_by_empty_value branch from 20e6b90 to d783be4 Compare November 4, 2020 11:49

SonataCI removed the pending author label Nov 4, 2020

VincentLanglet added the Needs: Upgrade note label Nov 4, 2020

peter-gribanov force-pushed the filter_by_empty_value branch from d783be4 to 8d13142 Compare November 9, 2020 11:26

peter-gribanov commented Nov 9, 2020

View reviewed changes

peter-gribanov force-pushed the filter_by_empty_value branch from 8d13142 to bf068fd Compare November 10, 2020 07:29

core23 requested changes Nov 10, 2020

View reviewed changes

peter-gribanov force-pushed the filter_by_empty_value branch from bf068fd to 398fa03 Compare November 10, 2020 10:09

VincentLanglet previously approved these changes Nov 10, 2020

View reviewed changes

core23 previously approved these changes Nov 10, 2020

View reviewed changes

core23 added patch and removed Needs: Upgrade note labels Nov 10, 2020

phansys requested changes Nov 10, 2020

View reviewed changes

UPGRADE-3.x.md Outdated Show resolved Hide resolved

UPGRADE-3.x.md Outdated Show resolved Hide resolved

UPGRADE-3.x.md Outdated Show resolved Hide resolved

UPGRADE-3.x.md Outdated Show resolved Hide resolved

delegate filter query by empty value to filters

c90dd44

extract code from Datagrid::buildPager() into separate methods to improve readability add upgrade info add example invert check to fail fast fix text in UPGRADE

peter-gribanov dismissed stale reviews from core23 and VincentLanglet via c90dd44 November 10, 2020 14:10

peter-gribanov force-pushed the filter_by_empty_value branch from 398fa03 to c90dd44 Compare November 10, 2020 14:10

phansys approved these changes Nov 10, 2020

View reviewed changes

VincentLanglet approved these changes Nov 10, 2020

View reviewed changes

VincentLanglet merged commit bb4b462 into sonata-project:3.x Nov 10, 2020

peter-gribanov deleted the filter_by_empty_value branch November 11, 2020 05:28

VincentLanglet mentioned this pull request Dec 12, 2020

Unchanged filters params #6700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegate filter query by empty value to filters #6402

Delegate filter query by empty value to filters #6402

peter-gribanov commented Sep 22, 2020

VincentLanglet commented Sep 22, 2020

peter-gribanov Sep 23, 2020

VincentLanglet Sep 23, 2020

SonataCI commented Oct 30, 2020

VincentLanglet commented Nov 4, 2020 •

edited

Loading

peter-gribanov Nov 9, 2020

VincentLanglet Nov 9, 2020

peter-gribanov Nov 10, 2020

VincentLanglet commented Nov 9, 2020

core23 Nov 10, 2020

peter-gribanov Nov 10, 2020

core23 Nov 10, 2020

core23 Nov 10, 2020

VincentLanglet commented Nov 10, 2020

		### Empty values in datagrid filters

		Now empty values are passed to datagrid filters. If you have custom datagrid filters, add empty string check to them.

Delegate filter query by empty value to filters #6402

Delegate filter query by empty value to filters #6402

Conversation

peter-gribanov commented Sep 22, 2020

Subject

Changelog

VincentLanglet commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonataCI commented Oct 30, 2020

VincentLanglet commented Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet commented Nov 9, 2020

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

VincentLanglet commented Nov 10, 2020

VincentLanglet commented Nov 4, 2020 •

edited

Loading