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

[REQUEST] Paginator to support multiple paginations in 1 page #164

Closed
johnnncodes opened this issue Jan 26, 2013 · 30 comments
Closed

[REQUEST] Paginator to support multiple paginations in 1 page #164

johnnncodes opened this issue Jan 26, 2013 · 30 comments

Comments

@johnnncodes
Copy link

Don't know if you guys will agree to me, but I think it would be really great if the paginator will support multiple paginations in 1 page. Have tried to do this in CI and L3 but I can't really make it to work. Even finding a solution for this in google is really hard. Some said they just resorted in using ajax calls to support multiple paginations in 1 page but I think its not a neat solution since it depends on javascript. So i think it would be really great if this feature will be added so devs can easily use multiple paginations in one page. Have seen many developers struggle implementing multiple paginations in 1 page including me.

@taylorotwell
Copy link
Member

Eh, this would be pretty complicated to implement since the paging would need to work independently for each paginator. More trouble than it's worth I think.

@JoostK
Copy link
Contributor

JoostK commented Jan 26, 2013

I actually did implement this in L3, the way I did it was to pass an additional parameter the the Paginator constructor which would be used as query variable. You then need to alter a couple of methods to also add this additional parameter, but that's quite easy really.

The code (for L3, not compatible with L4) can be found here but it actually contains a few more changes (it will preserve additional query variables). Also note that I needed to alter more methods in Eloquent for global support.

@johnnncodes
Copy link
Author

@JoostK - Thanks for sharing :) Does it work for like 3 or more paginations in 1 page? Or it has a limit of 2 maximum paginations in 1 page only? Another thing, If I have 3 paginations and I'm in the #2 page of paginator A then I go the #3 page of paginator B. Will it still preserve the current page of paginator A and paginator C? Thanks in advance.

Great solution btw, only problem is you need to alter Eloquent, which seems its not a very good idea since we will need to alter the core just to support multiple paginations on 1 page. That's why I asked for this feature request since its better if its included in the core so users won't need to apply their modifications to the core to make this work. Sad thing is this is closed even there is no need to rush this feature request, it can just be on a roadmap for future versions of Laravel 4.

@JoostK
Copy link
Contributor

JoostK commented Jan 26, 2013

The altered class works with an infinite amount of paginators on one page 😃 All paginator instances are independent and retrieve their page variable by the name you give, so if you specify a distinct name for all of your paginators, they will just work.

Current pages of other paginators will also be kept. That why I added append_query (which is called by default in the constructor), which copies over all query variables from the current request, so they will not be lost.

In L3 it isn't much of a problem to alter the core, I'm updating through Git so it will just merge in all changes and if a merge conflict occurs I just have to resolve it, but code will never get lost. This is not possible anymore with L4.

@johnnncodes
Copy link
Author

Yeah, I'm also thinking it allows infinite paginators on one page and page will be preserved since you are using query strings. Nice solution indeed!

But "This is not possible anymore with L4." - Why? :(

@JoostK
Copy link
Contributor

JoostK commented Jan 27, 2013

In L4, Composer is used to handle dependencies. The Laravel framework itself is a dependency of your application so it will be managed by Composer, not with Git. Maybe Composer updates using Git by merging upstream into local, but I assume it simply overwrites everything.

@johnnncodes
Copy link
Author

@JoostK - since you know how to implement multiple paginations in 1 page, if you are now familiar with the L4 architecture maybe you can contribute a pull request that implements multiple paginations in 1 page? And let's see if Taylor will approve it. If it will not break something and all is good, I think it will be merged to the core. Additional feature for the Paginator.

@johnnncodes
Copy link
Author

So far, I haven't seen a Framework yet that supports multiple paginations in a page. So I think it would really be great if L4 will be able to provide this feature, and many PHP devs will also benefit from it since they can use L4 components like the Paginator outside the framework I think.

@taylorotwell
Copy link
Member

They probably don't do it because it is overly complicated to implement and usually not worth the trouble.

@AlekzZz
Copy link

AlekzZz commented Jul 9, 2013

Tailor, why is it overly complicated you can't use JoostK solution or make something similar?
All cms such as wordpress / drupal, etc support multiple paginations on a single page, you just define unique pager id or it uses "page=2&page_1=3" and so on.

Is it really that overly complicated to allow custom page identifier and listen for changes on it?

That would be a really good addition to the great framework you made.

@ronaldcastillo
Copy link
Contributor

Even when composer is used to handle dependencies, you could just overwrite the alias and the provider in your config/app.php

@ollieread
Copy link
Contributor

Since this ended 4 months ago, I assume that this wasn't something that was added, nor is it planned to be added.

For anyone else out there who finds this while searching for answers, you can just create your own custom class that extends the core classes to adjust functionality in this way.

@dschu-lab
Copy link

I'm still interested in this, because I have multiple lists with items and want to paginate both, the lists & items on one page. I tried it with custom base urls, but had no luck with that. :/

@ollieread: Could you share your solution with us?

@freezedriedpop
Copy link

Although it's old i'll +1 this as well.

Surely the best way would just be the ability to set a custom get variable instead of "page" (defaults to page obviously). That way you could just use Foo::paginate(10,'foo_page'); Bar::paginate(10,'bar_page');

@taylorotwell taylorotwell reopened this Jan 29, 2014
@taylorotwell
Copy link
Member

Re-opening this. Plan to tackle it this week.

@taylorotwell
Copy link
Member

Can someone post how they implemented it?

@JoostK
Copy link
Contributor

JoostK commented Jan 29, 2014

I did for L3. Basically it involved passing an additional parameter as name (which will be the query param used). This also needs to be added in Eloquent. I also adjusted the way query params are appended, because with multiple paginators it is unnatural that switching pages forgets all other pages, so I altered it to always copy over all other query params.

The complexity is not that bad, but it may not really be expressive, considering the fact that the paginator already takes quite a few parameters, even when used from Eloquent. It is perhaps possible to use a setter or another more expressive way to accomplish this.

L3 implementation is here. Eloquent was altered like so

@JoostK
Copy link
Contributor

JoostK commented Jan 29, 2014

I see that the Paginator class itself may use a setter, it is not immediately required in the constructor, but postponed until setupPaginationContext, which is called from Environment::make, right after the constructor. So, Environment::make does need to know the name. Eloquent obviously also needs it right away.

I think it would be best to move Environment::$pageName to Paginator::$pageName, settable via a setter (perhaps optional as last argument to constructor). Query retrieval is done in Environment::getCurrentPage(), which should accept the page name as an argument. Then alias Environment::make and Query::paginate to e.g. Environment::named and Query::paginateAs to allow for the name to be specified as first argument.

@taylorotwell
Copy link
Member

If someone has any interest in making a pull request for this I will look at it.

@vgoodvin
Copy link

Please look at my solution #3868

@ollieread
Copy link
Contributor

Did this ever get rectified?

Ahh yes, I see that the setPageName() method was added, excellent.

@upngo
Copy link

upngo commented Jun 24, 2014

@ollieread I see the commit request above, but don't see this in master so assume it didn't get used?

@ollieread
Copy link
Contributor

I'm pretty sure somebody else committed as I definitely saw a way in the code.

@ollieread
Copy link
Contributor

Yeah, there we go!

@stacyvlasits
Copy link

@ollieread / @upngo
Did you actually get something working using that method? I see it in the codebase, but I don't see a way to use it.

@hkan
Copy link

hkan commented Aug 5, 2014

I accomplished the expected result through a pretty ugly way.

$paginated = new Receipt;

$paginated->getQuery()
          ->getConnection()
          ->getPaginator()
          ->setPageName('receipt_page');

return $paginated->paginate($limit);

Something like Receipt::paginate([ 'page_name' => 'receipt_page', 'limit' => $limit ]); would have been wonderful. Anybody willing to make the change? I would do it, but I still have trouble with writing tests so my PR probably won't get merged.

Edit: The paginate method of Eloquent returning a query builder instead of a collection might be a solution, too. This way, we can do this:

return Receipt::paginate($limit)->withPageName('receipt_page')->get();

What does anybody else think about this?

@ollieread
Copy link
Contributor

If this still isn't something that's truly working, I can take a look and make a PR for a fix.

@hkan
Copy link

hkan commented Aug 5, 2014

I couldn't find any better solution than the one at the beginning of my previous comment. There is Paginator::setPageName() method but this sets the name globally, and it's not what I want.

@stacyvlasits
Copy link

The fact that the Paginator::setPageName() sets the name globally required
a lot of back flips and a bunch of ugly code in order to manage the page
names and queries. In the end, I gave up trying to manage two paginators
on one page, which was probably the best design decision for both UI and
code anyway.

Stacy Vlasits
2057 Zach Scott Street
Austin, TX 78723
(330) 347-0836

On Tue, Aug 5, 2014 at 11:04 AM, Hakan AKTAS [email protected]
wrote:

I couldn't find any better solution than the one at the beginning of my
previous comment. There is Paginator::setPageName() method but this sets
the name globally, and it's not what I want.


Reply to this email directly or view it on GitHub
#164 (comment).

joelharkes pushed a commit to joelharkes/framework_old that referenced this issue Mar 7, 2019
Users table now includes the virtual email. Fixes laravel#163
gonzalom pushed a commit to Hydrane/tmp-laravel-framework that referenced this issue Oct 12, 2023
[phpunit]: delete unused syntaxCheck parameter
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

No branches or pull requests