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

Filter object #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Filter object #506

wants to merge 1 commit into from

Conversation

mabar
Copy link
Contributor

@mabar mabar commented Mar 24, 2021

Build conditions for findBy(), orderBy() and limitBy() via object

Example usage: (check tests for more)

$filter = $repository->createFilter();

$find = $filter->find();
$find->equal('property1', 'value');
$find->notEqual('property2', 'value');

$order = $find->order();
$order->property('property3', ICollection::DESC);

$filter->limit(50, 10);

$collection = $repository->findByByFilter($filter);

Pros:

  • type-safe and less error-prone than array
  • even in complex conditions will be IDE plugin able to hint property names
  • php methods as type-safe shortcuts for orm functions can be added

Cons:

  • ??

@mabar
Copy link
Contributor Author

mabar commented Mar 24, 2021

New API is not in IRepository intentionally, so it could be released before next major. Separate issue should be created for that.

Note to self:

  • Repository API should be probably also added to collections
  • Documentation

@mabar
Copy link
Contributor Author

mabar commented Mar 24, 2021

Also I am not sure whether it would be more helpful to add methods chaining or change ordering interface to something like $order->property('property3)->desc(). These two are both fluent interfaces and such are incompatible.

@hrach
Copy link
Member

hrach commented Mar 24, 2021

Hi, first of all - thanks for exploring this and trying to come up with solution. 👍

I would like to have something like this but TBH this is a low priority for me now. The solution should bring something other than "new query syntax" here. Specifically I'm thinking about:

  • better API for using custom collection functions (ideally typesafe, at least with PhpStan)
  • API for using an expression on the "right" side (i.e. doing COUNT(...) > COUNT(...) and so)

I was already thinking about something like IExpression but never finished the idea to some meaninful concept.

If this works for you nicely, definitely keep using it in your code, probably the missing part is the autoconversion to array condition but I'd say it's quite usable without that :)

@mabar
Copy link
Contributor Author

mabar commented Mar 24, 2021

better API for using custom collection functions (ideally typesafe, at least with PhpStan)

Currently it requires function to be class-string<IArrayFunction|IQueryBuilderFunction>, so it is checked by phpstan. Also for specific functions can be added separate methods, like $order->countAggregate('books->id'). Not sure what more can we do

API for using an expression on the "right" side

Yeah, it would be useful. I will try to come up with same api that would be compatible with that feature. Some more complex example would be helpful.

@hrach
Copy link
Member

hrach commented Mar 24, 2021

Also, I'm thinking about reverting the way what you do here:

  • make the object representation a default one and make array notation backward compatible and let it reparse it to these objects. - Then these object could do "more" than just wrapping around array.
  • at the same time we could delay processing these objects to the actually sql & array filtering until it is needed.

Just wanna let you know what I personally expect of this project - not asking you to do it. This definitely needs more exploration. Lately I find the proper API (for new dbal & orm features) the most difficult ask.

@mabar
Copy link
Contributor Author

mabar commented Mar 24, 2021

Let's say this is working proof of concept. Whether it uses objects or arrays under the hood doesn't matter :)
I would just like to know if you like the proposed filtration syntax and what should be these new potential features so we can find out whether api can be improved.
We could even do the internal transition from arrays to objects after release of filtration objects (making them final and marking few methods as internal should be enough).

@hrach
Copy link
Member

hrach commented Mar 24, 2021

I would just like to know if you like the proposed filtration syntax and what should be these new potential features so we can find out whether api can be improved.

Naming is not much important now. The important thing is e.g. composing multiple branches of condition - like you generate the conditions in different places and merge them later via AND.

We could even do the internal transition from arrays to objects after release of filtration objects

The underlying change may shape the resulting API much more than we can expect. This is the reason I see no hurry meting this when it may be a nice user-defined API.

@mabar
Copy link
Contributor Author

mabar commented Mar 24, 2021

e.g. composing multiple branches of condition - like you generate the conditions in different places and merge them later via AND.

So instead of

$find->and(static function (FindFilter $scope): void {
	$scope->greaterOrEqual('property', 10);
	$scope->lowerOrEqual('property', 20);
});

Something like this?

$condition[] = new GreaterOrEqual('property', 10);
$condition[] = new LowerOrEqual('property', 20);

$find->and($conditions);

I am sure we could eventualy mix it together, but I see no simple way how could be building subnodes and then the root node supported by phpstan and phpstorm plugins.

@mabar
Copy link
Contributor Author

mabar commented Apr 20, 2021

I was playing with that approach last few weeks and imho it could look like this:

$find->by(
  $find->notEqual(
    $find->countAggregate('triggers->id'),
    0,
  ),
);

by() method would be the only one writing something into the filter. Other methods would be just shortcuts for creating functions - so instead of $find->notEqual(... could be new CompareNotEqualsFunction(...

Composing multiple branches of condition should be fairly easy :)

For and/or conditions I see two viable approaches:

  • composing in a scoped function
$find->by($find->or(static function (FindFilter $scope) use ($guardedObjects): void {
  foreach ($guardedObjects as $code => $object) {
    $scope->by(
      $scope->equal('guardedObjectCode', (string) $code)
    );

    if ($object !== null) {
      $scope->by(
        $scope->equal('guardedObject', $object)
      );
    }
  }
}));
  • an array of conditions
$orConditions = [];
foreach ($guardedObjects as $code => $object) {
  $orConditions[] = $find->equal('guardedObjectCode', (string) $code);

  if ($object !== null) {
    $orConditions[] = $find->equal('guardedObject', $object);
  }
}
$find->by($find->or($orConditions));

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.

2 participants