-
-
Notifications
You must be signed in to change notification settings - Fork 957
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. |
| * @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
| * @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.