Fix Dupplication code on Filters

[Simperfit]

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	no
License	MIT
Doc PR	no

Since the last commit with both boolean and numeric filters, scrutinizer does not like duplicated code.

And a foreach that scrutinizer does not like.

[teohhanhui]

It's not necessary to specify pass by reference for $queryBuilder. Objects are always passed by reference.

[Simperfit]

Ohh, yes, I was thinking that this one was not passed by reference when testing this, but it was another problem, ill remove that

[teohhanhui]

Perhaps addJoinsForNestedProperty

[teohhanhui]

Also, please use type hints for parameters.

[theofidry]

IMO AbstractFilter is already too big and I don't see the point of having it in an abstract class. It should be separated as a doctrine filter utility instead...

[teohhanhui]

IMO AbstractFilter is already too big and I don't see the point of having it in an abstract class. It should be separated as a doctrine filter utility instead...

Wait for it... Traits 😉

[theofidry]

😱

[Simperfit]

What can we do about that, using Traits?

[theofidry]

What can we do about that, using Traits?

...

using Traits?

On a more serious note, this would be a utility for filters. As users are very likely to create custom filters and may need those utility methods, I think allowing them to a utility without any contract (which is what a trait is) would be a very bad idea. A good old class declared as a service do the job here.

And performance are not an issue, adding a class to the DIC doesn't cause more performance slowness than copy/pasting code left and right.

[Simperfit]

We should do that in this PR ?

[theofidry]

In another one IMO, it is a separate issue.

[dunglas]

I would prefer a trait. It's a perfect use case for a trait. Adding things like this to the DIC do slow down the app (but yes, for just 1 it's not signifiant).

[theofidry]

It's a perfect use case for a trait. Adding things like this to the DIC do slow down the app (but yes, for just 1 it's not signifiant).

It's not, you have to add over 1000 services to the Symfony DIC to see 1-2ms difference. It would only be a lazy shortcut to avoid declaring a service to the DIC and introduce an implicit contract, which is always harder to manage than an external one.

[theofidry]

Besides as you have to inject dependencies via a the constructor. Making it a trait is worse than having an abstract class at every level, even in terms of DX.

[teohhanhui]

// use ApiPlatform\Core\Exception\InvalidArgumentException;

/**
 * Adds the necessary joins for a nested property.
 *
 * @param string       $property
 * @param string       $rootAlias
 * @param QueryBuilder $queryBuilder
 *
 * @throws InvalidArgumentException If property is not nested
 *
 * @return array An array where the first element is the join $alias of the leaf entity,
 *               and the second element is the $field name
 */
protected function addJoinsForNestedProperty(string $property, string $rootAlias, QueryBuilder $queryBuilder) : array
{
    $propertyParts = $this->splitPropertyParts($property);

    if (0 === count($propertyParts['associations'])) {
        throw new InvalidArgumentException(sprintf('Cannot add joins for property "%s" - property is not nested.', $property));
    }

    $parentAlias = $rootAlias;

    ...
}

[Simperfit]

I don't think we need to throw an exception here, is the exception for the people who want to use this function ?

[theofidry]

to me looks like an abnormality, hence an exception to raise it. If the developer wants to ignore it, he can always catch that error, but if you ignore it, he has no means to know about it.

[Simperfit]

In our use cases the exception would not be thrown, in case the user want to use the methods without a nested property he will have to catch the error or/and to test with $this->isPropertyNested so it's ok to add the exception for me.

[dunglas]

👍

@Simperfit do you consider this PR ready to be merged?

[Simperfit]

@dunglas Yes, we will fix other scrutiner issues on equivalent PRs

[dunglas]

Thank you @Simperfit