-
-
Notifications
You must be signed in to change notification settings - Fork 921
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
Conversation
c528404
to
e34fbc4
Compare
I'm not sure of what I did for the MongoDbOdm part ;-) |
5a3a193
to
f117591
Compare
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) |
There was a problem hiding this comment.
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.
f117591
to
1db2b2d
Compare
Finally, by creating the behat tests, I realized that my code was not really doing what I expected.
Finally I’ll just keep the refactoring simplifying the creation of new strategies in the inherited classes. It was what I needed initially ;-) |
7818e75
to
d959323
Compare
* | ||
* @throws InvalidArgumentException If strategy does not exist | ||
*/ | ||
protected function addEqualityMatchStrategy(string $strategy, Builder $aggregationBuilder, string $matchField, $fieldType, $values, bool $caseSensitive) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ? :/
There was a problem hiding this comment.
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).
d959323
to
bcc543c
Compare
OK, so my original intention will lose its meaning in V3. ;-) |
c407e20
to
d6cad23
Compare
} | ||
|
||
/** | ||
* 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) |
There was a problem hiding this comment.
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.
d6cad23
to
ec51013
Compare
…asier to add new strategies to classes inheriting from SearchFilter class
ec51013
to
70e2893
Compare
Thanks @jpierront. |
All query manipulations are now located in the same method to ease the creation on classes inherited from SearchFilter.
Example:
We will be able to create easily a "not_int" strategy or more complexe cases.