Introduce FilterCollection#restore method

[VincentLanglet]

Closes #10536

I just don't know if it's better to introduce a new method or it should be the default behavior of the enable method.

[derrabus]

Thank you for the PR. I've retargeted it to the 2.15.x branch because your change clearly isn't a bugfix.

I understand your motivation behind the PR and realize that the vocabulary used by the methods (enable/disable) might be confusing in your case. However, I believe that this confusion is getting even worse with your patch because it suggests that a filter can be in a state that is neither enabled nor disabled and that state is even the default one for every filter.

Another problem is that with your patch applied, the configuration of a disabled filter will be kept (because I might want to restore it one fine day) and I have no way to free the memory occupied by it. In most cases, this might not actually cause an issue, but I'd still like to avoid such situations if possible.

How about adding a new method to suspend a filter which explicitly tells the FilterCollection that the caller intends to restore is later?

[VincentLanglet]

I like the idea of the suspend method. I'll make the change to not keep trace of a disabled filter and only keep trace of suspended filters.

I just need to handle the following behavior:

• If I suspend a filter then try to enable it (IMHO it will be overriden)
• If I suspend a filter then try to disable it

Also, I wonder if the "getFilter($name)" method will need to work on suspended filters...

[derrabus]

If I suspend a filter then try to enable it (IMHO it will be overriden)

Either that, or throw. But I think, we can implement it as you suggested: The suspended filter is deleted and a new one is created and enabled.

If I suspend a filter then try to disable it

Good question. I'd say throw because currently you cannot disable what isn't enabled and I'd like to keep it that way. Then again, how would I remove a suspended filter? 🤔 Ideas?

Also, I wonder if the "getFilter($name)" method will need to work on suspended filters...

Also a good question. Do we expect downstream code to assume that a given filter is active if getFilter() doesn't throw?

[VincentLanglet]

If I suspend a filter then try to enable it (IMHO it will be overriden)

Either that, or throw. But I think, we can implement it as you suggested: The suspended filter is deleted and a new one is created and enabled.

Done

If I suspend a filter then try to disable it

Good question. I'd say throw because currently you cannot disable what isn't enabled and I'd like to keep it that way.

Done

Then again, how would I remove a suspended filter? 🤔 Ideas?

With the current implementation, it's not possible... but I'm not sure it's needed.
When you suspend a filter, you're suppose to restore it later.
If you didn't want to restore it, you should have disable it instead.

Also, I wonder if the "getFilter($name)" method will need to work on suspended filters...

Also a good question. Do we expect downstream code to assume that a given filter is active if getFilter() doesn't throw?

The phpdoc say it only return enabled filters but doctrine code never use getFilter out of this class...

Still, since I don't expect someone to do

$filterCollection->suspend('foo');
$filterCollection->getFilter('foo')->setParameter('bar');

I don't think there is a need to get a suspended filter so I didn't change the method.

A suspended filter act like a disabled filter, you can't get it. The only diff is that you can restore it.

[derrabus]

Thanks. We should cover a few more scenarios with tests:

• The behavior of isDisabled(), enable(), disable(), has() and getFilter() for suspended filters.
• The behavior of isSuspended() for enabled, disabled, suspended and invalid filters.

[VincentLanglet]

Thanks. We should cover a few more scenarios with tests:

• The behavior of isDisabled(), enable(), disable(), has() and getFilter() for suspended filters.
• The behavior of isSuspended() for enabled, disabled, suspended and invalid filters.

has method is unrelated to enabled/suspended filters.

For all the others methods (and the getHash method) I should cover all the scenarios now.

[beberlei]

I believe this should be the default of enable()

[derrabus]

I believe this should be the default of enable()

Maybe it should've been.

How do we deal with apps that rely on calling disable followed by enable resets the parameters of a filter?

[VincentLanglet]

Yeah, my concern was that changing this for enable could be considered as a BC-break now...

Also, like noticed by @derrabus here, if we want enable to restore the previous filter, it means that we can't free the memory occupied with disable anymore.

[VincentLanglet]

So, should I change something in the enable/disable methods instead or is it ok this way @beberlei @derrabus ?

[derrabus]

From my PoV, the PR is okay as it is. Maybe @beberlei has a better idea how to address our concers regarding BC.

[VincentLanglet]

From my PoV, the PR is okay as it is. Maybe @beberlei has a better idea how to address our concers regarding BC.

Currently the state of the PR is "having requested change", can you approved it @derrabus if the PR is now okay for you ? :)

@beberlei, is this PR also okay for you ?

[derrabus]

I can approve it, but I'd still like to get a response from @beberlei.

[beberlei]

@derrabus @VincentLanglet maybe introduce a new method enableFilters (or other name that you come up with) that will be the default from now on and deprecate the enable() method?

[derrabus]

@derrabus @VincentLanglet maybe introduce a new method enableFilters (or other name that you come up with) that will be the default from now on and deprecate the enable() method?

So, basically a ->disable() call followed by a ->enableFilter() call would restore the previous configuration of that filter and in 3.0 we drop the old ->enable() method? The would force us to keep the configuration of a disabled filter, not knowing if we will ever need it. We've discussed this in an earlier iteration of this PR and the separate suspend()/restore() methods were the result of that discussion.

Questions regarding that approach:

• How would I reset the configuration of a filter? With that new workflow, I don't see an easy way to do that.
• How can I free the memory occupied by a filter configuration if I know that I won't need it again?

[VincentLanglet]

Questions regarding that approach:

• How would I reset the configuration of a filter? With that new workflow, I don't see an easy way to do that.
• How can I free the memory occupied by a filter configuration if I know that I won't need it again?

With these questions in mind, I find great the proposed implementation

• disable free the memory and reset the state
• suspend just "temporary disable"

[greg0ire]

Removing from the milestone until there is an agreement.

[VincentLanglet]

I'm not sure to understand the decision about this PR and what should I do now @derrabus @beberlei ?

[VincentLanglet]

What should be done here @greg0ire @derrabus @beberlei ?

[derrabus]

I've pinged @beberlei on Slack.

[beberlei]

Lets go with restore then

[derrabus]

Thank you. And sorry for the delay, @VincentLanglet.

[derrabus]

Damn, I merged to the wrong branch. Reverted on 2.15.x and cherry-picked onto 2.16.x as 64ee76e.