Allow filter strategies to set parameters (for invalidating the Doctrine cache) (Case 142739)

[janopae]

As discussed in #18 (comment), this PR provides a better way to invalidate the Doctrine cache when the filter SQL changes.

#18 Already provides a backwards-compatible fix that generates the full filter SQL and adds it as a parameter. This PR allows the FilterStrategies to set and retrieve their parameters themselves.

This presents a breaking change, as the interface for FilterStrategies is changed.

I didn't find a way to only set the parameters if needed. If I knew a hook to only execute code always before a database query is performed, I would have used it in the first place instead of relying on the OnRequestDependencyInjector.

• Provide an interface to set and retreive parameters from the FilterStrategy
• Make this interface work and replace the hotfix introduced in Hotfix: Consider result of FilterStrategy in Query Cache #18
• Fix tests
• Add test

[janopae]

@MalteWunsch ping

[janopae]

wf: Visibility-Filter-Bundle: Vernünftige Lösung für Parameter-Caching-Problem

[MalteWunsch]

Not so sure about that: $this->visibleValue has no type checks, so it could be a mutable object with a __toString() method. But maybe that is considered a misuse and out of scope for this PR. (Could be solved in a separate PR, e.g. with a typed constructor signature, if you like)

[janopae]

Having a mutable object with a __toString() method would require ignoring the @param annotation and passing a wrong type to the constructor. So I would indeed consider it a misuse.

Would you consider passing a wrong type according to the docblock as something to be officially supported? That would mean that changing a DockBlock type declaration into a PHP type declaration was a breaking change.

The constructor uses a dockblock type declaration because it's a union type, and union types got introduced in PHP 8. This library however also supports PHP >=7.2. So we sadly can't turn it into a PHP type declaration until we drop support for PHP 7.

[MalteWunsch]

If this is a misuse, the // not needed comment is fine with me.

I just would like to make accidental misuse harder. If you want to support PHP 7.2, what about something like this?

    /**
     * @param string|int|bool $visibleValue
     */
    public function __construct($visibleValue)
    {
        if (!is_string($visibleValue) && !is_int($visibleValue) && !is_bool($visibleValue)) {
          throw new InvalidArgumentException();
        }
        $this->visibleValue = $visibleValue;
    }

[MalteWunsch]

"undesired consequences" and "please" seem too vague for my taste. Would you mind explaining it in more detail (or simply remove the unprecise wording)?

[sebastiankugler]

which will have undesired consequences!

suggestion: which will have undesired consequences such as objects being visible for users who should not see them. (no acclamation mark)

Adds all parameters to the collection

seems to contradict

So please add all parameters to the collection.

Either the method adds all parameters or the user has to do it. Maybe "Adds the parameters" would be better, as it avoids using the identical wording.

So please add all parameters to the collection.

Suggestion: To avoid this, make sure that all parameters are added to the collection.

[MalteWunsch]

This class feels a bit weird to me: it cannot be used without an SQLFilter and only delegates to its methods of the name. While it's nice to have the ParameterCollection as an interface to state what exactly is needed, I'm not sure if this is worthwhile?

[janopae]

This is one of the reasons I waited for a review – I hoped you had a strong opinion on this.

Removing the class and the interface is always easier than re-introducing it, and I think the interface makes the bundle easier to use and avoids mistakes.

On the other hand, the interface should probably be adjusted when the Doctrine SQLFilter interface changes, making this just duplicated code.

Do we need a third opinion on this?

[janopae]

I just realised that I forgot to add support for setParameterList to the interface/the SQLFilterAsParamterCollection class. Removing the interface and using the SQLFilter directly would avoid mistakes like this in the first place.

[MalteWunsch]

Still no strong oppinion here, but I've given it some more thought:

You want to declare the type of the parameter for FilterStrategy->addParameters($parameters). If you would declare it as SQLFilter, the FilterStrategy interface becomes tightly coupled to the SQLFilter. Which you probably want to avoid, as exchanging the FilterStrategy is your bundle's ultimate extension point.

To preserve the freedom you currently have for FilterStrategy implementations, you should keep the ParameterCollection interface (and therefore the SQLFilterAsParameterCollection implementation).

I'm not entirely convinced that the FilterStrategy is a good extension point - maybe I lack the imagination for a useful non trivial implementation without an SQLFilter. But as I consider the FilterStrategy central for the bundle, my bottom line suggestion is: keep the ParameterCollection and SQLFilterAsParameterCollection 👍

[janopae]

You want to declare the type of the parameter for FilterStrategy->addParameters($parameters). If you would declare it as SQLFilter, the FilterStrategy interface becomes tightly coupled to the SQLFilter. Which you probably want to avoid, as exchanging the FilterStrategy is your bundle's ultimate extension point.

To preserve the freedom you currently have for FilterStrategy implementations, you should keep the ParameterCollection interface (and therefore the SQLFilterAsParameterCollection implementation).

I'm not entirely convinced that the FilterStrategy is a good extension point - maybe I lack the imagination for a useful non trivial implementation without an SQLFilter.

Keeping the FilterStrategy Interface decoupled from the SQLFilter was indeed a reason why I introduced this interface, but it was not because I actually wanted to allow any other implementations to be used. Using other implementations than the SQLFilter-based SQLFilterAsParameterCollection is actually not even possible using the current implementation of this bundle, as the code that calls addParameters creates an instance of this class.

The ParameterCollection interface doesn't add any freedom to users of this bundle (except in unit tests, where the interface indeed eases mocking or the creation of test implementations). FilterStrategys that don't work together with an SQLFilter behind the scenes are not possible using this bundle.

The ParameterCollection interface was rather introduced for restraining the freedom of the implementor. I wanted to disallow calling any other methods on the SQLFilterthan the ones needed for managing parameters. Also, I thought that working with an SQLFilter instance in a FilterStrategy might be confusing if you don't understand the internals of this bundle. Even if you do understand the bundle, I think it would be odd (as the FilterStrategy is there to be called by an SQLFilter implementation, not to call it).

[janopae]

I'm still not sure whether these reasons suffice as an argument for the added complexity, or whether additional documentation is needed in order to make sure that people who try to understand the implementation don't stumple over this.