Skip to content
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

Open
wants to merge 6 commits into
base: 1.5
Choose a base branch
from

Conversation

GregoireHebert
Copy link

@GregoireHebert GregoireHebert commented Feb 13, 2018

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

Add a filtering system based on api platform work.
To begin with I have implemented the boolean filter.

Todo:

  • Update doc
  • Add search filter
  • Add numeric filter
  • Add exists filter
  • Add range filter
  • Add order filter

Copy link
Member

@lchrusciel lchrusciel left a 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;


Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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>
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

redundant

}

/**
* Applies the filters.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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)
Copy link
Member

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

@GregoireHebert
Copy link
Author

Thanks for your time ! AH ah no Kevin doesn't mind :)
In fact with him, we thought about a way to provide a plugin to add an api-platform support directly instead of me rewriting everything for this specific purpose. I don't know if he already took the time to realize a poc for it, and if not I definitely will pretty soon :)

@lchrusciel
Copy link
Member

If you need some help or explanation, you can always try to reach me on Sylius or Symfony slack.
Good to know, that you are thinking about it :) I was planning to go to Symfony Live for Kevin's API platform workshop, but I won't be able to manage it... Anyway, this collaboration could be beneficial for our communities.

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

Successfully merging this pull request may close these issues.

3 participants