-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat (filter): add filter system. #256
base: 1.5
Are you sure you want to change the base?
Conversation
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.
First of all: Thanks for your tremendous work!
Your implementation is great. It follows many best practices. You have used strict types and code is well documented. Also, it is great, that we can learn and share some implementation with API platform. It is awesome!
But on the other hand, many classes are really complex. I know that is the result of libraries limitation, but I'm afraid that I won't be able to provide proper support for this logic in the long term.
What would you say about creating another library on a top of this one, with this implementation? It will be a pity if we lose all this work. Also, a separate library may convince other contributors involved in API platform to help us maintain the project. And it will be a good alternative for people who don't want to use elastic search in their applications.
I really regrate, that I'm not able to help you because of lack of time :(
$conditions = ['boolean'=>['enabled'=>true]]; | ||
$resourceClass = Product::class; | ||
|
||
|
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.
redundant blank line
$this->applyFilter($conditions, $resourceClass, $queryBuilder); | ||
} | ||
|
||
function it_should_apply_a_boolean_condition(ManagerRegistry $managerRegistry, ObjectManager $om, ClassMetadata $classMetadata, LoggerInterface $logger, QueryBuilder $queryBuilder) |
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 would be more readable if we use full name instead of abbreviation for 'om'
* | ||
* @author Grégoire Hébert <gregoire@les-tilleuls.coop> | ||
*/ | ||
class FiltersDefinitionPass implements CompilerPassInterface |
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.
missing final
/** | ||
* Gets every filter definition and adds them to the Filter Extension. | ||
* | ||
* @author Grégoire Hébert <gregoire@les-tilleuls.coop> |
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.
I hope you don't mind, but we have resigned from author blocks as they are redundant (you will be mentioned in code contributors) and not obvious to maintain.
* | ||
* @author Grégoire Hébert <gregoire@les-tilleuls.coop> | ||
*/ | ||
class InvalidArgumentException extends \InvalidArgumentException |
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.
A little bit redundant ;)
} | ||
|
||
/** | ||
* Add a filter |
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.
redundant
} | ||
|
||
/** | ||
* Applies the filters. |
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.
redundant
* | ||
* @see https://github.com/api-platform/core for the canonical source repository | ||
* | ||
* @copyright Copyright (c) 2015-present Kévin Dunglas |
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.
I hope Kevin will not mind ;)
* @author Amrouche Hamza <hamza.simperfit@gmail.com> | ||
* @author Teoh Han Hui <teohhanhui@gmail.com> | ||
*/ | ||
class BooleanFilter extends AbstractFilter |
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.
WDYT about making this class final?
* | ||
* @throws \invalidArgumentException | ||
*/ | ||
// protected function applyFilter(string $property, $value, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = 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.
Should be removed I suppose
Thanks for your time ! AH ah no Kevin doesn't mind :) |
If you need some help or explanation, you can always try to reach me on Sylius or Symfony slack. |
Add a filtering system based on api platform work.
To begin with I have implemented the boolean filter.
Todo: