Calling FilterCollection::disable then FilterCollection::enable is losing the filter parameters. #7110

New Issue

admin · 2026-01-22T15:44:52+01:00

admin commented

2026-01-22 15:44:52 +01:00

Originally created by @VincentLanglet on GitHub (Feb 23, 2023).

Bug Report

Q	A
BC Break	no
Version	any

Summary

I'm not sure if it can be considered as a bug or if it's the expected behavior.

Let's say I have a deleted filter.

When using

$this->em->getFilters()->enable('deleted');
$this->em->getFilter('deleted')->setParameterList(...);
$this->em-> // do somequery

$this->em->getFilters()->disable('deleted);
$this->em-> // do somequery
$this->em->getFilters()->enable('deleted');

$this->em-> // do somequery

I would have expected that the last query used the Filter with the passed parameter list.

But I saw nothing in the doc or in the unit test of the FilterCollection saying if enable should work in the current behavior or in a restore way.

My use case is that the two first line

$this->em->getFilters()->enable('deleted');
$this->em->getFilter('deleted')->setParameterList(...);

are made in a Kernel request subscriber, and then in my project I either do

$this->em-> // do somequery

either do

$this->em->getFilters()->disable('deleted);
$this->em-> // do somequery
$this->em->getFilters()->enable('deleted');

for some special queries.

Expected behavior

Basically I would have expect the enable method to work like a restore one:

If an old definition of the filter exists use it
Else, create a new one

I see different solution for this:

the enable method should have work as a restore one, I can do the PR.
the enable method works as expected, but adding a restore method is a nice idea, I can do the PR.
there is nothing to change to the FilterCollection and the solution I see would be to add a disabled parameter to all my filter, and to change

$this->em->getFilters()->disable('deleted);
$this->em-> // do somequery
$this->em->getFilters()->enable('deleted');

by

$this->em->getFilter('deleted')->setParameter('disabled', true);
$this->em-> // do somequery
$this->em->getFilter('deleted')->setParameter('disabled', false);

I personally think the first or second options are the best ones.
I open to do a PR, just would like to know which solution is preferred (should it be a bugfix of enable or a new feature restore).

Originally created by @VincentLanglet on GitHub (Feb 23, 2023). ### Bug Report | Q | A |------------ | ------ | BC Break | no | Version | any #### Summary I'm not sure if it can be considered as a bug or if it's the expected behavior. Let's say I have a `deleted` filter. When using ``` $this->em->getFilters()->enable('deleted'); $this->em->getFilter('deleted')->setParameterList(...); $this->em-> // do somequery $this->em->getFilters()->disable('deleted); $this->em-> // do somequery $this->em->getFilters()->enable('deleted'); $this->em-> // do somequery ``` I would have expected that the last query used the Filter with the passed parameter list. But I saw nothing in the doc or in the unit test of the FilterCollection saying if `enable` should work in the current behavior or in a `restore` way. My use case is that the two first line ``` $this->em->getFilters()->enable('deleted'); $this->em->getFilter('deleted')->setParameterList(...); ``` are made in a Kernel request subscriber, and then in my project I either do ``` $this->em-> // do somequery ``` either do ``` $this->em->getFilters()->disable('deleted); $this->em-> // do somequery $this->em->getFilters()->enable('deleted'); ``` for some special queries. #### Expected behavior Basically I would have expect the `enable` method to work like a `restore` one: - If an old definition of the filter exists use it - Else, create a new one I see different solution for this: - the `enable` method should have work as a `restore` one, I can do the PR. - the `enable` method works as expected, but adding a `restore` method is a nice idea, I can do the PR. - there is nothing to change to the FilterCollection and the solution I see would be to add a `disabled` parameter to all my filter, and to change ``` $this->em->getFilters()->disable('deleted); $this->em-> // do somequery $this->em->getFilters()->enable('deleted'); ``` by ``` $this->em->getFilter('deleted')->setParameter('disabled', true); $this->em-> // do somequery $this->em->getFilter('deleted')->setParameter('disabled', false); ``` I personally think the first or second options are the best ones. I open to do a PR, just would like to know which solution is preferred (should it be a bugfix of `enable` or a new feature `restore`).

admin closed this issue

2026-01-22 15:44:52 +01:00

admin commented

2026-01-22 15:44:52 +01:00

@VincentLanglet commented on GitHub (Feb 23, 2023):

By looking at the enable method

public function enable($name)
    {
        if (! $this->has($name)) {
            throw new InvalidArgumentException("Filter '" . $name . "' does not exist.");
        }

        if (! $this->isEnabled($name)) {
            $filterClass = $this->config->getFilterClassName($name);

            assert($filterClass !== null);

            $this->enabledFilters[$name] = new $filterClass($this->em);

            // Keep the enabled filters sorted for the hash
            ksort($this->enabledFilters);

            $this->setFiltersStateDirty();
        }

        return $this->enabledFilters[$name];
    }

There is also the following behavior

$this->em->getFilters()->enable('deleted');
$this->em->getFilter('deleted')->setParameterList(...);
$this->em->getFilters()->disable('deleted);
$this->em->getFilters()->enable('deleted');

// Here the filter doesn't have the parameter list anymore

$this->em->getFilters()->enable('deleted');
$this->em->getFilter('deleted')->setParameterList(...);
$this->em->getFilters()->enable('deleted');

// Here the filter still have the parameter list (the enable method basically did nothing)

This might be kinda confusing, I wonder if calling enable on a already enabled filter shouldn't trigger a deprecation and throw an exception in next major instead of doing nothing. 🤔

@VincentLanglet commented on GitHub (Feb 23, 2023): By looking at the `enable` method ``` public function enable($name) { if (! $this->has($name)) { throw new InvalidArgumentException("Filter '" . $name . "' does not exist."); } if (! $this->isEnabled($name)) { $filterClass = $this->config->getFilterClassName($name); assert($filterClass !== null); $this->enabledFilters[$name] = new $filterClass($this->em); // Keep the enabled filters sorted for the hash ksort($this->enabledFilters); $this->setFiltersStateDirty(); } return $this->enabledFilters[$name]; } ``` There is also the following behavior ``` $this->em->getFilters()->enable('deleted'); $this->em->getFilter('deleted')->setParameterList(...); $this->em->getFilters()->disable('deleted); $this->em->getFilters()->enable('deleted'); // Here the filter doesn't have the parameter list anymore $this->em->getFilters()->enable('deleted'); $this->em->getFilter('deleted')->setParameterList(...); $this->em->getFilters()->enable('deleted'); // Here the filter still have the parameter list (the enable method basically did nothing) ``` This might be kinda confusing, I wonder if calling enable on a already enabled filter shouldn't trigger a deprecation and throw an exception in next major instead of doing nothing. 🤔

admin commented

2026-01-22 15:44:53 +01:00

@mpdude commented on GitHub (Feb 26, 2023):

Personally, I find it not all too surprising:

enable enables the filter, if it is not already enabled
disable disables it, and there remain no traces of the filter on the particular query.

We have to take care that we do not break BC, i. e. users who disable() a filter currently have to assume/expect that enable will not bring back the old parameters, and we cannot change that.

If I were you, I'd go with the additional disabled flag that you can implement in your filter yourself, since that also makes you independent of a fix here and/or the version it will be released with.

@mpdude commented on GitHub (Feb 26, 2023): Personally, I find it not all too surprising: * `enable` enables the filter, if it is not already enabled * `disable` disables it, and there remain no traces of the filter on the particular query. We have to take care that we do not break BC, i. e. users who `disable()` a filter currently have to assume/expect that `enable` will _not_ bring back the old parameters, and we cannot change that. If I were you, I'd go with the additional `disabled` flag that you can implement in your filter yourself, since that also makes you independent of a fix here and/or the version it will be released with.

admin commented

2026-01-22 15:44:53 +01:00

@VincentLanglet commented on GitHub (Feb 26, 2023):

Thanks for your answer.

Indeed, without any change in the doctrine code, I'll go with my "disabled" parameter.

But I still opened a PR to add a restore method with tests and docs. Do you think it can be something valuable to merge ?

@VincentLanglet commented on GitHub (Feb 26, 2023): Thanks for your answer. Indeed, without any change in the doctrine code, I'll go with my "disabled" parameter. But I still opened a PR to add a restore method with tests and docs. Do you think it can be something valuable to merge ?

admin referenced this issue

2026-01-22 16:07:09 +01:00

[PR #7110] Drop tools/sandbox #10317

admin referenced this issue

2026-01-22 16:07:10 +01:00

[PR #7114] Remove tools from PHPCs #10322

admin referenced this issue

2026-01-22 16:10:04 +01:00

[PR #8859] Drop tools/sandbox #11214

Sign in to join this conversation.