search

sonata-project / SonataDoctrineORMAdminBundle

Integrate Doctrine ORM into the SonataAdminBundle
https://docs.sonata-project.org/projects/SonataDoctrineORMAdminBundle
MIT License
445 stars 345 forks source link

ModelAutocompleteFilter "is not equal to" and NULL database value #1040

Closed jean-gui closed 3 years ago

jean-gui commented 4 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.65.0 3.65.0 The missing Symfony Admin Generator
sonata-project/block-bundle              3.18.4 3.18.4 Symfony SonataBlockBundle
sonata-project/cache                     2.0.1  2.0.1  Cache library
sonata-project/core-bundle               3.18.0 3.18.0 Symfony SonataCoreBundle (abandoned)
sonata-project/datagrid-bundle           2.5.0  3.2.0  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.6.0  1.6.0  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.17.1 3.17.1 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                  2.2.0  2.2.0  Lightweight Exporter library

Symfony packages

$ composer show --latest 'symfony/*'
Restricting packages listed in "symfony/symfony" to "4.4.*"
symfony/asset                      v4.4.7  v4.4.7  Symfony Asset Component
symfony/browser-kit                v4.4.7  v4.4.7  Symfony BrowserKit Component
symfony/cache                      v4.4.7  v4.4.7  Symfony Cache component with PSR-6, PSR-16, and tags
symfony/cache-contracts            v2.0.1  v2.0.1  Generic abstractions related to caching
symfony/config                     v4.4.7  v4.4.7  Symfony Config Component
symfony/console                    v4.4.7  v4.4.7  Symfony Console Component
symfony/css-selector               v4.4.7  v4.4.7  Symfony CssSelector Component
symfony/debug                      v4.4.7  v4.4.7  Symfony Debug Component
symfony/debug-bundle               v4.4.7  v4.4.7  Symfony DebugBundle
symfony/dependency-injection       v4.4.7  v4.4.7  Symfony DependencyInjection Component
symfony/doctrine-bridge            v4.4.7  v4.4.7  Symfony Doctrine Bridge
symfony/dom-crawler                v4.4.7  v4.4.7  Symfony DomCrawler Component
symfony/dotenv                     v4.4.7  v4.4.7  Registers environment variables from a .env file
symfony/error-handler              v4.4.7  v4.4.7  Symfony ErrorHandler Component
symfony/event-dispatcher           v4.4.7  v4.4.7  Symfony EventDispatcher Component
symfony/event-dispatcher-contracts v1.1.7  v2.0.1  Generic abstractions related to dispatching event
symfony/expression-language        v4.4.7  v4.4.7  Symfony ExpressionLanguage Component
symfony/filesystem                 v4.4.7  v4.4.7  Symfony Filesystem Component
symfony/finder                     v4.4.7  v4.4.7  Symfony Finder Component
symfony/flex                       v1.6.2  v1.6.2  Composer plugin for Symfony
symfony/form                       v4.4.7  v4.4.7  Symfony Form Component
symfony/framework-bundle           v4.4.7  v4.4.7  Symfony FrameworkBundle
symfony/http-foundation            v4.4.7  v4.4.7  Symfony HttpFoundation Component
symfony/http-kernel                v4.4.7  v4.4.7  Symfony HttpKernel Component
symfony/inflector                  v4.4.7  v4.4.7  Symfony Inflector Component
symfony/intl                       v4.4.7  v4.4.7  A PHP replacement layer for the C intl extension that includes additional data from the ICU library.
symfony/lock                       v4.4.7  v4.4.7  Symfony Lock Component
symfony/mime                       v4.4.7  v4.4.7  A library to manipulate MIME messages
symfony/monolog-bridge             v4.4.7  v4.4.7  Symfony Monolog Bridge
symfony/monolog-bundle             v3.5.0  v3.5.0  Symfony MonologBundle
symfony/options-resolver           v4.4.7  v4.4.7  Symfony OptionsResolver Component
symfony/orm-pack                   v1.0.8  v1.0.8  A pack for the Doctrine ORM
symfony/phpunit-bridge             v4.4.7  v5.0.7  Symfony PHPUnit Bridge
symfony/polyfill-intl-grapheme     v1.15.0 v1.15.0 Symfony polyfill for intl's grapheme_* functions
symfony/polyfill-intl-icu          v1.15.0 v1.15.0 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-intl-idn          v1.15.0 v1.15.0 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-intl-normalizer   v1.15.0 v1.15.0 Symfony polyfill for intl's Normalizer class and related functions
symfony/polyfill-mbstring          v1.15.0 v1.15.0 Symfony polyfill for the Mbstring extension
symfony/polyfill-php72             v1.15.0 v1.15.0 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-php73             v1.15.0 v1.15.0 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions
symfony/process                    v4.4.7  v4.4.7  Symfony Process Component
symfony/profiler-pack              v1.0.4  v1.0.4  A pack for the Symfony web profiler
symfony/property-access            v4.4.7  v4.4.7  Symfony PropertyAccess Component
symfony/routing                    v4.4.7  v4.4.7  Symfony Routing Component
symfony/security                   v4.4.7  v4.4.7  Symfony Security Component
symfony/security-acl               v3.0.4  v3.0.4  Symfony Security Component - ACL (Access Control List)
symfony/security-bundle            v4.4.7  v4.4.7  Symfony SecurityBundle
symfony/serializer                 v4.4.7  v4.4.7  Symfony Serializer Component
symfony/service-contracts          v2.0.1  v2.0.1  Generic abstractions related to writing services
symfony/stopwatch                  v4.4.7  v4.4.7  Symfony Stopwatch Component
symfony/string                     v5.0.7  v5.0.7  Symfony String component
symfony/swiftmailer-bundle         v3.4.0  v3.4.0  Symfony SwiftmailerBundle
symfony/templating                 v4.4.7  v4.4.7  Symfony Templating Component
symfony/test-pack                  v1.0.6  v1.0.6  A pack for functional and end-to-end testing within a Symfony app
symfony/translation                v4.4.7  v4.4.7  Symfony Translation Component
symfony/translation-contracts      v2.0.1  v2.0.1  Generic abstractions related to translation
symfony/twig-bridge                v4.4.7  v4.4.7  Symfony Twig Bridge
symfony/twig-bundle                v4.4.7  v4.4.7  Symfony TwigBundle
symfony/validator                  v4.4.7  v4.4.7  Symfony Validator Component
symfony/var-dumper                 v4.4.7  v4.4.7  Symfony mechanism for exploring and dumping PHP variables
symfony/var-exporter               v4.4.7  v4.4.7  A blend of var_export() + serialize() to turn any serializable data structure to plain PHP code
symfony/web-profiler-bundle        v4.4.7  v4.4.7  Symfony WebProfilerBundle
symfony/webpack-encore-bundle      v1.7.3  v1.7.3  Integration with your Symfony app & Webpack Encore!
symfony/workflow                   v4.4.7  v4.4.7  Symfony Workflow Component
symfony/yaml                       v4.4.7  v4.4.7  Symfony Yaml Component

PHP version

$ php -v
PHP 7.3.11-0ubuntu0.19.10.4 (cli) (built: Apr  8 2020 18:58:29) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.11, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.3.11-0ubuntu0.19.10.4, Copyright (c) 1999-2018, by Zend Technologies
    with Xdebug v2.7.2, Copyright (c) 2002-2019, by Derick Rethans

Subject

I have several filters such as

$datagridMapper->add(
                'user',
                ModelAutocompleteFilter::class,
                [],
                null,
                [ 'property'  => ['value', 'family', 'given', 'emails.value']]
            )

When using it with "is not equal to", results don't include entries for which the user column is null, which is very counter-intuitive. I think that's somewhat related to https://github.com/sonata-project/SonataAdminBundle/issues/3569 and #991, although not exactly the same.

I managed to fix this by replacing the line 107 of ModelAutocompleteFilter: before:

$this->applyWhere($queryBuilder, sprintf('%s != :%s', $alias, $parameterName));

after:

$this->applyWhere($queryBuilder, sprintf('%s IS NULL OR %s != :%s', $alias, $alias, $parameterName));

But I don't know if that's the best way to fix it or if this will create other issues. I also don't know the code enough to assess that possibility and provide a usable MR.

Thanks, Jean-Gui

VincentLanglet commented 4 years ago

When using it with "is not equal to", results don't include entries for which the user column is null, which is very counter-intuitive.

I'm not sure it's counter-intuitive. For me this is the behaviour of every Filter provided by Sonata. When you look for a value which is not equal to something, you still look for a value.

It's similar to the behaviour of SQL:

SELECT * from foo WHERE bar <> 'something'

Doesn't return the foo with a NULL bar value.

The change introduced by the issue #991 was kinda to use IS NULL if you selected null value specifically, instead of == NULL which was always false or not filtering at all.

What you're asking would be a feature or a BC-break, it doesn't seems like a bug to me.

jean-gui commented 4 years ago

The confusing thing is that because of this behavior the total number of rows is different from the sum of "is equal to" and "is not equal to". At least that's confusing to my user and to me since I had to dig to understand what was going on.

If you think it's not a bug, could it at least be configurable? That would would also prevent the BC-break.

VincentLanglet commented 4 years ago

When you're looking for a boolean value which is not true, you get the same result that when you're looking for a boolean which is false. You don't get the null value. I'm pretty sure changing this behaviour would lead to someone saying that's confusing.

This is the same in your case but with multiple value. Let say there is 4 values, A, B, C, D. Not A = B + C + D.

When you use the DateFilter and looking for a date which is not between yesterday and today, null date are not return.

But there seems to be a custom NULL operator:

public const CHOICES = [
        DateOperatorType::TYPE_EQUAL => '=',
        DateOperatorType::TYPE_GREATER_EQUAL => '>=',
        DateOperatorType::TYPE_GREATER_THAN => '>',
        DateOperatorType::TYPE_LESS_EQUAL => '<=',
        DateOperatorType::TYPE_LESS_THAN => '<',
        DateOperatorType::TYPE_NULL => 'NULL',
        DateOperatorType::TYPE_NOT_NULL => 'NOT NULL',
    ];

This could be an idea...

VincentLanglet commented 4 years ago

Hi @jean-gui

I found https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/649/files https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/650/files

It seems like filters was starting to work like you wanted.

I looked again at the filter

@sonata-project/contributors I think we should look for consistency. Does a negative operator should return null values or not ?

It currently weird that ModelFilter does, but ModelAutocompleteFilter doesn't. Or that StringFilter does but ChoiceFilter or NumberFilter doesn't.

jean-gui commented 4 years ago

Thanks @VincentLanglet for investigating this issue!

VincentLanglet commented 4 years ago

Do you want to make a PR @jean-gui ?

jean-gui commented 4 years ago

Unfortunately, I don't think I have enough time for that :-/

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

VincentLanglet commented 3 years ago

Closing in favor of https://github.com/sonata-project/SonataDoctrineORMAdminBundle/issues/1513