Skip to content

SearchFilter: Refactor code to reduce code duplication and makes it easier to add new strategies to classes inheriting from SearchFilter class #3541

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

Merged
merged 2 commits into from
Mar 3, 2021

Conversation

jpierront
Copy link
Contributor

@jpierront jpierront commented May 1, 2020

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tickets
License MIT
Doc PR api-platform/docs#...

All query manipulations are now located in the same method to ease the creation on classes inherited from SearchFilter.

Example:

class ExtendedSearchFilter extends SearchFilter
{
    public const STRATEGY_OTHER = 'other';

    protected function addWhereByStrategy(string $strategy, QueryBuilder $queryBuilder, string $alias, string $field, $fieldType, $values, bool $caseSensitive, string $valueParameter)
    {
        if (self::STRATEGY_OTHER === $strategy) {
            // ...

            return;
        }

        parent::addWhereByStrategy($strategy, $queryBuilder, $alias, $field, $fieldType, $values, $caseSensitive, $valueParameter);
    }
}

We will be able to create easily a "not_int" strategy or more complexe cases.

@jpierront jpierront force-pushed the jpierront-patch-1 branch from c528404 to e34fbc4 Compare May 1, 2020 20:51
@jpierront
Copy link
Contributor Author

I'm not sure of what I did for the MongoDbOdm part ;-)

@jpierront jpierront force-pushed the jpierront-patch-1 branch 2 times, most recently from 5a3a193 to f117591 Compare May 1, 2020 21:10
@alanpoulain
Copy link
Member

Seems nice! Could you add a Behat test to validate its behavior? It will allow to make sure MongoDB works too.

}

/**
* Adds where clause according to the strategy.
*
* @throws InvalidArgumentException If strategy does not exist
*/
protected function addWhereByStrategy(string $strategy, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $field, $fieldType, $value, bool $caseSensitive)
protected function addWhereByStrategy(string $strategy, QueryBuilder $queryBuilder, string $alias, string $field, $fieldType, array $values, bool $caseSensitive, string $valueParameter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a BC break.

@jpierront jpierront force-pushed the jpierront-patch-1 branch from f117591 to 1db2b2d Compare May 2, 2020 12:35
@jpierront jpierront changed the title SearchFilter: Allow strategy on associations SearchFilter: Refactor code to reduce code duplication and makes it easier to add new strategies to classes inheriting from SearchFilter class May 2, 2020
@jpierront
Copy link
Contributor Author

Finally, by creating the behat tests, I realized that my code was not really doing what I expected.

  • Doing a "partial" search on "relatedDumies.name" was already working before my PR.
  • Doing a "partial" search on "relatedDumies" (So on the identifier) is not accepted by Doctrine ORM (It doesn't accept to make a "LIKE" on identifier in this case)

Finally I’ll just keep the refactoring simplifying the creation of new strategies in the inherited classes. It was what I needed initially ;-)

@jpierront jpierront force-pushed the jpierront-patch-1 branch 5 times, most recently from 7818e75 to d959323 Compare May 2, 2020 15:08
*
* @throws InvalidArgumentException If strategy does not exist
*/
protected function addEqualityMatchStrategy(string $strategy, Builder $aggregationBuilder, string $matchField, $fieldType, $values, bool $caseSensitive)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the class is final, it should be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we allow to extends the ORM version but not the ODM one ? :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Because the ORM version will also be a final class in the next major version (3.0).

@jpierront jpierront force-pushed the jpierront-patch-1 branch from d959323 to bcc543c Compare May 2, 2020 19:24
@jpierront
Copy link
Contributor Author

OK, so my original intention will lose its meaning in V3. ;-)
My refactoring still seems interesting to centralize the code related to the strategy in the same place.

@jpierront jpierront force-pushed the jpierront-patch-1 branch from c407e20 to d6cad23 Compare May 2, 2020 19:37
}

/**
* Adds where clause according to the strategy.
*
* @throws InvalidArgumentException If strategy does not exist
*/
protected function addWhereByStrategy(string $strategy, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $field, $fieldType, $value, bool $caseSensitive)
protected function addWhereByStrategy(string $strategy, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $field, $fieldType, $values, bool $caseSensitive, string $valueParameter = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even like this, it's a BC break. You should mimic what has been done here: #3521.

Base automatically changed from master to main January 23, 2021 21:59
@alanpoulain alanpoulain changed the base branch from main to 2.6 March 3, 2021 09:59
…asier to add new strategies to classes inheriting from SearchFilter class
@alanpoulain alanpoulain merged commit 9df5a5e into api-platform:2.6 Mar 3, 2021
@alanpoulain
Copy link
Member

Thanks @jpierront.

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

Successfully merging this pull request may close these issues.

2 participants