Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Advanced filter value lost when changing default value type #6865

Closed
kappaj opened this issue Feb 16, 2021 · 24 comments · Fixed by #6871 or #6877
Closed

Advanced filter value lost when changing default value type #6865

kappaj opened this issue Feb 16, 2021 · 24 comments · Fixed by #6871 or #6877
Labels

Comments

@kappaj
Copy link

kappaj commented Feb 16, 2021

Environment

Sonata packages

show

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.89.0 3.89.0 The missing Symfony Admin Generator
sonata-project/block-bundle              4.5.0  4.5.0  Symfony SonataBlockBundle
sonata-project/cache                     2.0.1  2.1.1  Cache library
sonata-project/datagrid-bundle           3.3.0  3.3.0  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.11.0 1.11.0 Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.29.0 3.29.0 Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                  2.5.1  2.5.1  Lightweight Exporter library
sonata-project/form-extensions           1.9.0  1.9.0  Symfony form extensions
sonata-project/twig-extensions           1.5.1  1.5.1  Sonata twig extensions
sonata-project/user-bundle               4.9.0  4.11.0 Symfony SonataUserBundle

Symfony packages

show

$ composer show --latest 'symfony/*'
symfony/apache-pack                v1.0.1  v1.0.1  A pack for Apache support in Symfony
symfony/asset                      v4.4.19 v5.2.3  Manages URL generation and versioning of web assets such as CSS stylesheets, JavaScript files and image files
symfony/browser-kit                v4.4.19 v5.2.3  Simulates the behavior of a web browser, allowing you to make requests, click on links and submit forms programm...
symfony/cache                      v4.4.19 v5.2.3  Provides an extended PSR-6, PSR-16 (and tags) implementation
symfony/cache-contracts            v2.2.0  v2.2.0  Generic abstractions related to caching
symfony/config                     v4.4.19 v5.2.3  Helps you find, load, combine, autofill and validate configuration values of any kind
symfony/console                    v4.4.19 v5.2.3  Eases the creation of beautiful and testable command line interfaces
symfony/css-selector               v4.4.19 v5.2.3  Converts CSS selectors to XPath expressions
symfony/debug                      v4.4.19 v4.4.19 Provides tools to ease debugging PHP code
symfony/debug-bundle               v4.4.19 v5.2.3  Provides a tight integration of the Symfony Debug component into the Symfony full-stack framework
symfony/debug-pack                 v1.0.9  v1.0.9  A debug pack for Symfony projects
symfony/dependency-injection       v4.4.19 v5.2.3  Allows you to standardize and centralize the way objects are constructed in your application
symfony/deprecation-contracts      v2.2.0  v2.2.0  A generic function and convention to trigger deprecation notices
symfony/doctrine-bridge            v4.4.19 v5.2.3  Provides integration for Doctrine with various Symfony components
symfony/dom-crawler                v4.4.19 v5.2.3  Eases DOM navigation for HTML and XML documents
symfony/dotenv                     v4.4.19 v5.2.3  Registers environment variables from a .env file
symfony/error-handler              v4.4.19 v5.2.3  Provides tools to manage errors and ease debugging PHP code
symfony/event-dispatcher           v4.4.19 v5.2.3  Provides tools that allow your application components to communicate with each other by dispatching events and l...
symfony/event-dispatcher-contracts v1.1.9  v2.2.0  Generic abstractions related to dispatching event
symfony/expression-language        v4.4.19 v5.2.3  Provides an engine that can compile and evaluate expressions
symfony/filesystem                 v4.4.19 v5.2.3  Provides basic utilities for the filesystem
symfony/finder                     v4.4.19 v5.2.3  Finds files and directories via an intuitive fluent interface
symfony/flex                       v1.12.1 v1.12.1 Composer plugin for Symfony
symfony/form                       v4.4.19 v5.2.3  Allows to easily create, process and reuse HTML forms
symfony/framework-bundle           v4.4.19 v5.2.3  Provides a tight integration between Symfony components and the Symfony full-stack framework
symfony/http-client-contracts      v2.3.1  v2.3.1  Generic abstractions related to HTTP clients
symfony/http-foundation            v4.4.19 v5.2.3  Defines an object-oriented layer for the HTTP specification
symfony/http-kernel                v4.4.19 v5.2.3  Provides a structured process for converting a Request into a Response
symfony/inflector                  v4.4.19 v5.2.3  Converts words between their singular and plural forms (English only)
symfony/intl                       v4.4.19 v5.2.3  Provides a PHP replacement layer for the C intl extension that includes additional data from the ICU library
symfony/maker-bundle               v1.29.1 v1.29.1 Symfony Maker helps you create empty commands, controllers, form classes, tests and more so you can forget about...
symfony/mime                       v4.4.19 v5.2.3  Allows manipulating MIME messages
symfony/monolog-bridge             v4.4.19 v5.2.3  Provides integration for Monolog with various Symfony components
symfony/monolog-bundle             v3.6.0  v3.6.0  Symfony MonologBundle
symfony/options-resolver           v4.4.19 v5.2.3  Provides an improved replacement for the array_replace PHP function
symfony/orm-pack                   v1.2.0  v2.1.0  A pack for the Doctrine ORM
symfony/phpunit-bridge             v5.2.3  v5.2.3  Provides utilities for PHPUnit, especially user deprecation notices management
symfony/polyfill-intl-grapheme     v1.22.1 v1.22.1 Symfony polyfill for intl's grapheme_* functions
symfony/polyfill-intl-icu          v1.22.1 v1.22.1 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-intl-idn          v1.22.1 v1.22.1 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-intl-normalizer   v1.22.1 v1.22.1 Symfony polyfill for intl's Normalizer class and related functions
symfony/polyfill-mbstring          v1.22.1 v1.22.1 Symfony polyfill for the Mbstring extension
symfony/polyfill-php72             v1.22.1 v1.22.1 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-php73             v1.22.1 v1.22.1 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions
symfony/polyfill-php80             v1.22.1 v1.22.1 Symfony polyfill backporting some PHP 8.0+ features to lower PHP versions
symfony/profiler-pack              v1.0.5  v1.0.5  A pack for the Symfony web profiler
symfony/property-access            v4.4.19 v5.2.3  Provides functions to read and write from/to an object or array using a simple string notation
symfony/property-info              v4.4.19 v5.2.3  Extracts information about PHP class' properties using metadata of popular sources
symfony/routing                    v4.4.19 v5.2.3  Maps an HTTP request to a set of configuration variables
symfony/security-acl               v3.1.1  v3.1.1  Symfony Security Component - ACL (Access Control List)
symfony/security-bundle            v4.4.19 v5.2.3  Provides a tight integration of the Security component into the Symfony full-stack framework
symfony/security-core              v4.4.19 v5.2.3  Symfony Security Component - Core Library
symfony/security-csrf              v4.4.19 v5.2.3  Symfony Security Component - CSRF Library
symfony/security-guard             v4.4.19 v5.2.3  Symfony Security Component - Guard
symfony/security-http              v4.4.19 v5.2.3  Symfony Security Component - HTTP Integration
symfony/serializer                 v4.4.19 v5.2.3  Handles serializing and deserializing data structures, including object graphs, into array structures or other f...
symfony/service-contracts          v2.2.0  v2.2.0  Generic abstractions related to writing services
symfony/stopwatch                  v4.4.19 v5.2.3  Provides a way to profile code
symfony/string                     v5.2.3  v5.2.3  Provides an object-oriented API to strings and deals with bytes, UTF-8 code points and grapheme clusters in a un...
symfony/swiftmailer-bundle         v3.5.2  v3.5.2  Symfony SwiftmailerBundle
symfony/templating                 v4.4.19 v5.2.3  Provides all the tools needed to build any kind of template system
symfony/translation                v4.4.19 v5.2.3  Provides tools to internationalize your application
symfony/translation-contracts      v2.3.0  v2.3.0  Generic abstractions related to translation
symfony/twig-bridge                v4.4.19 v5.2.3  Provides integration for Twig with various Symfony components
symfony/twig-bundle                v4.4.19 v5.2.3  Provides a tight integration of Twig into the Symfony full-stack framework
symfony/validator                  v4.4.19 v5.2.3  Provides tools to validate values
symfony/var-dumper                 v4.4.19 v5.2.3  Provides mechanisms for walking through any arbitrary PHP variable
symfony/var-exporter               v4.4.19 v5.2.3  Allows exporting any serializable PHP data structure to plain PHP code
symfony/web-profiler-bundle        v4.4.19 v5.2.3  Provides a development tool that gives detailed information about the execution of any request
symfony/webpack-encore-bundle      v1.11.0 v1.11.0 Integration with your Symfony app & Webpack Encore!
symfony/yaml                       v4.4.19 v5.2.3  Loads and dumps YAML files

PHP version

$ php -v
PHP 7.4.10 (cli) (built: Sep  1 2020 16:52:21) ( NTS Visual C++ 2017 x64 )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.10, Copyright (c), by Zend Technologies

Subject

I have a admin list view with a default filter with activated advanced filter.

Here is my code to define the filters and default filter:

show

protected function configureDatagridFilters(DatagridMapper $datagridMapper)
    {
        $datagridMapper
            ->add('chatState',
                'doctrine_orm_choice', [
                    'global_search' => true,
                    'field_type'    => ChoiceType::class,
                    'field_options' => [
                        'choices' => ChatStates::getChoices()
                    ]
                ]
            );
    }

protected function configureDefaultFilterValues(array &$filterValues)
    {
        $filterValues['chatState'] = [
            'type'  => EqualOperatorType::TYPE_NOT_EQUAL,
            'value' => 'archived',
        ];
    }

When I change the filter type to 'equal' and press the 'Filter' the list should filter all items with chatState = 'archived'

Tested with sonata-admin verion 3.89. Have to step back to <3.86 to get old filter behavior.

Minimal repository with the bug

Steps to reproduce

Expected results

  • List is filtered with chatState = 'archived'
  • url params should look like this: list?filter[chatState][type]=1&filter[chatState][value]=archived

Actual results

  • List is empty
  • url params are: list?filter[chatState][type]=1
    -> value is missing
@kirya-dev
Copy link
Contributor

Hello! Im not agree with expected results.

  1. You define this:
      $filterValues['chatState'] = [
            'type'  => EqualOperatorType::TYPE_NOT_EQUAL,
            'value' => 'archived',
        ];

But expects equals condition.
2) Default filters has already in your source code. If we pass all params into url its make url is too long.

@kappaj
Copy link
Author

kappaj commented Feb 16, 2021

Hi kirye-dev. I am not sure if I explained it right. The default filter works fine. I open the list view first time and the list is filtered correctly: chatState != archived.
But then I change the type to Equal in the frontend (to filter it chatState = archived) and press Filter. And then the unexpected result occurs.
It looks like the value "archived" (defined in the default filter) is empty when I "overwrite" the type via get params.

@kirya-dev
Copy link
Contributor

kirya-dev commented Feb 16, 2021

Hm. Its incorrect usage!

You must:

  1. Use in admin $this->getFilterParameters() instead directly get query data from request
  2. Or use $data['value'] in https://sonata-project.org/bundles/doctrine-orm-admin/master/doc/reference/filter_field_definition.html#callback in configuration of chatState datagrid filter

@kappaj
Copy link
Author

kappaj commented Feb 16, 2021

  1. Where should I use $this->getFilterParameters()? I don't think I get query data from request somewhere directly. Isn't it the default usage to implement configureDatagridFilters and configureDefaultFilterValues and sonata grep the query params internally?
  2. So I have to use always a callback? Why does it work before?

@VincentLanglet
Copy link
Member

@kirya-dev I confirm the bug

$filterValues['usagename'] = [
            'type' => ContainsOperatorType::TYPE_CONTAINS,
            'value' => 'a',
        ];

Is a valid way to declare a default filter value.

With this I get
image

If I change to
image

And I submit, you can notice that the a is disabled.
image

So the page reload with only the type loosing the value.
image

And, If I want to filter back on the default value, I can't. If I write
image

When I submit it's automatically disabled
image

So I still get the previous filter
image

@VincentLanglet
Copy link
Member

Same happen for a filter without default value, If I use it
image

And then I want to remove the value
image

When I submit it's disabled
image

So on the page reload, I still have the value
image

For this reason, if I try to remove the default filter on usagename
image

On submit fields are disabled, so on page reload, it automatically come back
image

@kappaj
Copy link
Author

kappaj commented Feb 16, 2021

I think I have it.

Line 712 in Sonata\AdminBundle\Admin\AbstractAdmin:

$parameters = array_merge($parameters, $filters);

But I think it must be:

$parameters = array_merge_recursive($parameters, $filters);

Otherwise the $filters array overwrite the property chatState.

@VincentLanglet
Copy link
Member

To me, it's more the following criteria which is wrong

// Compare default and submitted filter values in `keyValue` representation. (`keyValue` ex: "filter[publish][value][end]=2020-12-12")
// Only allow to submit non default and non empty values, because empty values means they are not present.
var changedValues = submittedValues.filter(function (keyValue) {
    return defaultValues.indexOf(keyValue) === -1 && keyValue.split('=')[1] !== '';
});

We have to compare the value vs the default value

@kirya-dev
Copy link
Contributor

kirya-dev commented Feb 16, 2021

$parameters = array_merge_recursive($parameters, $filters);

Hm. Current and your sudgest code dident worked correctly:

$data = ['id' => ['type' => 1, 'value' => 'foo']];
$data2 =  ['id' => ['type' => 2]]; // from request

var_dump(
	array_merge($data, $data2),
	array_merge_recursive($data, $data2)
);

output

array(1) {
  ["id"]=>
  array(1) {
    ["type"]=>
    int(2)
  }
}
array(1) {
  ["id"]=>
  array(2) {
    ["type"]=>
    array(2) {
      [0]=>
      int(1)
      [1]=>
      int(2)
    }
    ["value"]=>
    string(3) "foo"
  }
}

@VincentLanglet
Copy link
Member

For the js, something like this should be done:

var defaultValues = $.param({'filter': JSON.parse(this.dataset.defaultValues)}).split('&'),
                submittedValues = $form.serialize().split('&');

            var defaultValuesByKey = [];
            defaultValues.forEach(function (keyValue) {
                defaultValuesByKey[keyValue.split('=')[0]] = keyValue.split('=')[1];
            })

            // Compare default and submitted filter values in `keyValue` representation. (`keyValue` ex: "filter[publish][value][end]=2020-12-12")
            // Only allow to submit non default and non empty values, because empty values means they are not present.
            // An empty value should be submitted if the filter has a default value, in order to override it
            var changedValues = submittedValues.filter(function (keyValue) {
                if (defaultValuesByKey[keyValue.split('=')[0]]) {
                    return defaultValuesByKey[keyValue.split('=')[0]] !== keyValue.split('=')[1]
                } else {
                    return keyValue.split('=')[1] !== '';
                }
            });

@VincentLanglet
Copy link
Member

With both the JS modified and the array_merge modified I think the issue will be solved.

@kirya-dev
Copy link
Contributor

kirya-dev commented Feb 16, 2021

No @VincentLanglet. Problem in backend only. Fronted works correctly.

Its can be covered by phpunit

@kappaj
Copy link
Author

kappaj commented Feb 16, 2021

I got a right result with array_replace_recursive

            dump($parameters);
            dump($filters);
            $parameters = array_replace_recursive($parameters, $filters);
            dd($parameters);

abstract

@kirya-dev
Copy link
Contributor

Your right. Im test array_merge_recursive

@kirya-dev
Copy link
Contributor

But arrays values not correctly will merged with replace

@VincentLanglet
Copy link
Member

No @VincentLanglet. Problem in backend only. Fronted works correctly.

I'm sorry, but I took one hour to test every possible cases, I take screenshot and I provide a solution.
So yes, the problem is both backend AND frontend.

Because you disable the empty value you cannot remove a default filter.
You have to disable the empty value ONLY IF it's not a filter with a default value.

Your right. Im test array_merge_recursive

array_merge_recursive works for the case provided.

But If I change the value from a to b, I will end with an array ['a', 'b'] so it won't work.

@kirya-dev
Copy link
Contributor

kirya-dev commented Feb 16, 2021

Im says in another words: Need submit only CHANGED filters. Empty check needed for that didnt present in defaultValues

@kappaj
Copy link
Author

kappaj commented Feb 16, 2021

But arrays values not correctly will merged with replace

Do you have en example?

@VincentLanglet
Copy link
Member

Im says in another words: Need submit only CHANGED filters. Empty check needed for that didnt present in defaultValues

And that's not what's done by the current JS.

var changedValues = submittedValues.filter(function (keyValue) {
    return defaultValues.indexOf(keyValue) === -1 && keyValue.split('=')[1] !== '';
});

If my default value is usagename=a, and I submit an empty usagename, it will submit usagename=.
So defaultValues.indexOf(keyValue) will be -1 and keyValue.split('=')[1] will be ''. Your check will remove the empty value I try to submit in order to remove the default filter.

It's because you looking for usagename= in an array containing usagename=a, when you should look for the key usagename only.

@willemverspyck
Copy link
Contributor

Do you have en example?

@kappaj: For example a ChoiceType when you use "multiple" => "true":

$parameters:

^ array:5 [▼
  "_sort_order" => "DESC"
  "_sort_by" => "id"
  "status" => array:1 [▼
    "value" => array:3 [▼
      0 => "ACCEPT"
      1 => "DECLINE"
      2 => "REVIEW"
    ]
  ]
]

$filters:

^ array:3 [▼
  "status" => array:1 [▼
    "value" => array:3 [▼
      0 => "ACCEPT"
      1 => "DECLINE"
    ]
  ]
  "_page" => 1
  "_per_page" => 25
]

With "array_replace_recursive" it will be:

^ array:7 [▼
  "_sort_order" => "DESC"
  "_sort_by" => "id"
  "status" => array:1 [▼
    "value" => array:3 [▼
      0 => "ACCEPT"
      1 => "DECLINE"
      2 => "REVIEW"
    ]
  ]
  "_page" => 1
  "_per_page" => 25
]

But it must be:

^ array:7 [▼
  "_sort_order" => "DESC"
  "_sort_by" => "id"
  "status" => array:1 [▼
    "value" => array:3 [▼
      0 => "ACCEPT"
      1 => "DECLINE"
    ]
  ]
  "_page" => 1
  "_per_page" => 25
]

I think there is no PHP function for this, because on the third depth of the array you don't want to use "recursive" anymore. You have to use something like this:

function mergeParameters(array $parameters, array $filters): array {
    $data = [];

    foreach ($parameters as $key => $parameter) {
        $replace = array_key_exists($key, $filters) ? $filters[$key] : $parameter;

        if (is_array($parameter)) {
            $data[$key] = array_replace($parameter, $replace);
        } else {
            $data[$key] = $replace;
        }
    }

    return $data;
}

$parameters = mergeParameter($parameters, $filters);

I can make PR when #6871 is merged. It will be good to test this "mergeParameters" method to define all different situations.

@VincentLanglet
Copy link
Member

I can make PR when #6871 is merged. It will be good to test this "mergeParameters" method to define all different situations.

You can also make a PR on the PR#6871. It will avoid us to merge a non-finished PR.
Indeed, adding test could be great.

@kirya-dev
Copy link
Contributor

@willemverspyck your example incorrect: any data from filters will be ignored

@willemverspyck
Copy link
Contributor

You're correct @kirya-dev. I was to fast :-)

But I will make an PR on the PR#6871 with some tests.

This was referenced Feb 17, 2021
@willemverspyck
Copy link
Contributor

Sorry, was struggling with the CS tests. But the PR with tests are ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants