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

Pagination functionality. #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

kikoseijo
Copy link
Contributor

Check this out. (Haven't tested, maybe needs a fix)

My recommendations its to remove all $perPage, references. clean all that and just just the new function to read directly from request this 2 vars: limit + page.

What I have seen in other libraries its people gives options to set this vars names, I don't think its necessary, you can always overwrite them.

Its up to you, my suggestions:

  • Clean, remove all $paginate & $perPage
  • move functions specs to the contract and just use the, {@inheritdoc}
  • show the contract in the readme for people references.

Simple the better.

@OzanKurt
Copy link
Owner

You need to limit the database query too. So your $query->get() approach is wrong. Also I think people can pass in the item limit from controller easily. No need to overkill a simple thing.

    public function index(Request $request) {
        $this->repo->paginate($request->limit);
    }

You approach also restricts a users ability to use limit as a request parameter while using pagination.

@kikoseijo
Copy link
Contributor Author

Humm...

$this->repo->paginate() its Laravel defaults.

This pagination implementation needs to know how many records are.
People can do $request->limit = 'xXX'

There is a simplePagination that does not require to know that.

Works well on Lumen, where defaults pagination was not giving right links.
By reading request, we don't need to pass by this vars, but you are the boss here!😋

@kikoseijo kikoseijo closed this Nov 26, 2017
@OzanKurt
Copy link
Owner

We can work this out for lumen for sure, it just needs to be cleaned up and go the correct place. We might make a class for lumen or a trait for lumen methods.

@OzanKurt OzanKurt reopened this Nov 26, 2017
@kikoseijo
Copy link
Contributor Author

Laravels magic resolves the page parameter from the $request, same we doing with limit.
The trick we adding here its to be able to turn pagination on-off by sending the limit in the request.
What this function does its the same than Laravel, but also returns others parameters in the query for the next_page_url and so..

make a test, for example with simple url , check what happens with rest of parameters when you do the paginate using laravel defaults.

Also, have a look at this:: https://github.com/rlacerda83/lumen-api-query-parser/wiki/Usage
its very interesting how parses urls and returns a query build.

with a url like this:

http://musicboxx.dev/v1/role?page=1&order=33&limit=3

Laravel returns:

    "first_page_url": "http://musicboxx.dev/v1/role?page=1",
    "from": 1,
    "last_page": 2,
    "last_page_url": "http://musicboxx.dev/v1/role?page=2",
    "next_page_url": "http://musicboxx.dev/v1/role?page=2",
    "path": "http://musicboxx.dev/v1/role",
    "per_page": 15,
    "prev_page_url": null,
    "to": 15,
    "total": 22

but I think this is more convenient:

    "first_page_url": "http://musicboxx.dev/v1/role?page=1&order=33&limit=3",
    "from": 1,
    "last_page": 8,
    "last_page_url": "http://musicboxx.dev/v1/role?page=8&order=33&limit=3",
    "next_page_url": "http://musicboxx.dev/v1/role?page=2&order=33&limit=3",
    "path": "http://musicboxx.dev/v1/role",
    "per_page": "3",
    "prev_page_url": null,
    "to": 3,
    "total": 22

@johannesschobel
Copy link

i would suggest to not "bind" this to specific request parameter (e.g., ?limit=xxx or ?page=xxx) as this would result in not being able to use this package with specific standards (e.g., json:api) that define other parameters..

I think, these query parameters should be customizable in the config file!

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

Successfully merging this pull request may close these issues.

3 participants