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

Call filter at default #10

Closed
karneaud opened this issue Sep 5, 2018 · 14 comments

Comments

@karneaud
Copy link

commented Sep 5, 2018

Is there a way to trigger the filter other than from request parameters?

@Kyslik

This comment has been minimized.

Copy link
Owner

commented Sep 6, 2018

This answers the question you asked:

Is there a way to trigger the filter other than from request parameters?

I think you should be able to do following:

// get filter instance somehow // resolve or just inject it
$filter->setBuilder(Model::query())->recent(); // returns Illuminate\Database\Eloquent\Builder

Example from tinker:

resolve(App\Filters\UserFilter::class)->setBuilder(App\User::query())->state('new')->toSql();
"select * from `users` where exists (select * from `states` where `users`.`state_id` = `states`.`id` and `slug` = ?)"

Where filter looks like:

...
function filterMap(): array
{
    return ['state' => ['status', 'state']];
}


function state($type = null)
{
    if (is_null($type)) {
        // show all
        return $this->builder;
    }

    return $this->builder->whereHas('state', function (Builder $query) use ($type) {
        $query->where('slug', $type);
    });
}

After reading title of the issue, I think you want to have some kind of default filter thats applied when no request data is present (configurable), is that correct?

If above examples does not solve what you want, explain the use case?
Something like I would like to be able to use following code which should do XYZ.

@Kyslik Kyslik changed the title call filter at default Call filter at default Sep 6, 2018

@karneaud

This comment has been minimized.

Copy link
Author

commented Sep 10, 2018

Yeah ...wanting to have a default setting when no request data is present. Will try this out. thanks!

@karneaud karneaud closed this Sep 10, 2018

@Kyslik Kyslik self-assigned this Sep 10, 2018

@Kyslik Kyslik reopened this Sep 10, 2018

@Kyslik

This comment has been minimized.

Copy link
Owner

commented Sep 10, 2018

@karneaud I wonder what would be better, setting the default in the filter class itself or in the controller.

I can imagine that this would look nice:

// in the controller
return $user->filter($filters, ['recent' => 'some parameter for the recent'])->paginate();
@karneaud

This comment has been minimized.

Copy link
Author

commented Sep 10, 2018

@Kyslik OMG... I was just looking at it and trying something out lol! I like this!

@karneaud

This comment has been minimized.

Copy link
Author

commented Sep 10, 2018

@Kyslik is it that would need to pass parameters to applyFilters method?

@Kyslik

This comment has been minimized.

Copy link
Owner

commented Sep 10, 2018

@karneaud if I code this (not sure when, maybe in the end of the week / during the week) all you will have to do is

return $user->filter($filters, ['recent' => 'some parameter for the recent'])->paginate();

I do not follow your second question

is it that would need to pass parameters to applyfilter() method?

@karneaud

This comment has been minimized.

Copy link
Author

commented Sep 10, 2018

@Kyslik

When calling the proposed method ->filter($filters, [....]) for scoped Method I'm using 1.0.2 with php 7

function scopeFilter(Builder, $filters, $defaults) 
{
    $this->apply($builder, $default);
}

public function apply(Builder $builder, $defaults = [])
{
    return $this->setBuilder($builder)->applyFilters($defaults)->getBuilder();
}

protected function applyFilters($defaults = [])
{
    $this->builderPresent();

    foreach (array_merge($this->filters(), $defaults) as $filter => $value) {
        if (method_exists($this, $filter)) {
            $this->builder = (is_null($value)) ? $this->$filter() : $this->$filter($value);
            ....
@Kyslik

This comment has been minimized.

Copy link
Owner

commented Sep 10, 2018

I am not sure how this will be implemented yet, maybe I will introduce new property $defaults so programmer may choose between setting the default per filter or in the controller, I will see.

@phylaxis

This comment has been minimized.

Copy link

commented Sep 19, 2018

Curious, did this get implemented because I need this exact same feature.

@Kyslik

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2018

@phylaxis @karneaud I have some non committed work on my local computer and thats about it. I stopped working on it because I was thinking that URL (query string) should represent the filters used:

Programmer is responsible for how filters are used - example:

  • /users - some default filter is applied lets say verified

now end-user clicks around and changes to a /users?recent now as a programmer you have to solve a dilemma: Do I show recent only or verified & recent, from this point you can see that if you pick the latter you need to make redirect to include this in query strings /users?recent&verified, because without it, its going to show also unverified users.

I think instead of default filters, programmers should simply redirect to the URL with a filter if they want to have the default filter, its inevitable either way.

I am welcome to any feedback on my thoughts above.

@phylaxis

This comment has been minimized.

Copy link

commented Sep 20, 2018

Yes, I see your point @Kyslik. In my case it's a little different because the "default" parameter would always be applied unless it's value was changed. Everything else is additive. But you're right in this can be handled in the controller. In my case I ended up doing something like this:

//if scheduled filter exists then use it, if not default

if( \Request::has('scheduled') )
{
   $campaign = Campaign::with(array(
      'episodes' => function($query) use ($filter) { $query->sortable(['scheduled' => 'asc'])->filter($filter)->orderBy('title', 'asc'); }
      )
   )->find($id);

   if( strtotime( \Request::input('scheduled') ) )
   {
      $date_filter = \Request::input('scheduled');
   }

} else {
   $campaign = Campaign::with(array(
      'episodes' => function($query) use ($date_filter) { $query->sortable(['scheduled' => 'asc'])->where('scheduled', '>=', $date_filter )->orderBy('title', 'asc'); }
      )
   )->find($id);
}

So I look for the existance of the parameter and if it's there use it, otherwise I use my "default" as a simple "where" statement for the first load. Then I use that in view my view to set the "default" value for the form i'm using to submit the filter options. Future submits will always include the default or changed version of that value. After the first load I know that all future loads should (unless the URL is manipulated) include a value for that filter parameter.

@Kyslik

This comment has been minimized.

Copy link
Owner

commented Sep 21, 2018

@phylaxis I am thinking on how to make the code you presented to read easier, because I think its messy only because of the missing functionality we are talking about.

Now that I realised how a real controller looks without the functionality, I definitely want to support default filters, just implement it a bit different than on sortable package, because I want to support auto-redirects if the corresponding query is missing (configurable) and make it implicit what the code does. Maybe something like:

public function index(Request $request, UserFilter $filter)
{
    $filter->default(['scheduled']); // auto-redirects if the filter is not in the URL already, obviously would work only on GET requests.
    //or
    $filter->applyDefault(...);

or filter may redirect, while container is resolving the filter if its possible (similarly how FormRequests throw up if validation fails). That would mean filter itself takes care of the default and is not configurable from within the controller which is a con.

or not so implicit approach, the 2nd parameter for $query->filter(..., ['recent']) scope.

Or all three.

@phylaxis

This comment has been minimized.

Copy link

commented Sep 21, 2018

Thanks for looking into this further. Good luck.

Kyslik added a commit that referenced this issue Sep 30, 2018
Kyslik added a commit that referenced this issue Oct 1, 2018
Simplify applying defaults
- improve test suite
- add `hasAnyFilter` macro on request
- #10
Kyslik added a commit that referenced this issue Oct 1, 2018
Refactor `default filters` functionality
- improve test-suite
- improve readme
- #10
Kyslik added a commit that referenced this issue Oct 1, 2018
Implement #10
- add `hasAnyFilter` macro on request
- improve test suite
- update readme
- update changelog
- remove Laravel 5.5 from Travis-CI configuration file
@Kyslik

This comment has been minimized.

Copy link
Owner

commented Oct 1, 2018

@Kyslik Kyslik closed this Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.