fuelphp-storage / fuelphp

FuelPHP framework
http://fuelphp.com
MIT License
274 stars 57 forks source link

Pagination class #30

Open WanWizard opened 11 years ago

WanWizard commented 11 years ago

Can be taken largely as-is from 1.x.

sagikazarmark commented 9 years ago

MIght worth a look at Pagerfanta. We just need to write some drivers around it (probably mainly for databas/ORM queries). Does its job pretty smoothly.

WanWizard commented 9 years ago

We have to be careful with this.

Remember we're building a development framework here, not a one-off website.

The more dependencies we have with products from others, the more problems we will have supporting our product. We can't say to our users "sorry your application broke, but we didn't write pagination, we borrowed it from someone else".

For each of the external products we include in the framework, we have to be prepared to know the code in-depth, and to fork their repo's and fix issues ourselfs in case of problems.

So unless there is a very strong case, I would be against this.

WanWizard commented 9 years ago

Furthermore, I don't like the architecture of this package.

Pagination is not about I/O, it's about generation a bit of HTML. If you would like pagination to be generation a complete page (table + pagination), it's more logical to do that via a Fieldset renderer, if only because there are a million and one ways to render tabular output.

@stevewest, comments?

sagikazarmark commented 9 years ago

I think the Pagination is not the most important part of the framework. It is a plain output formatting which can be done thousand ways. I generally agree with you that the framework need to provide some components, and shouldn't rely on third-party. (Eg. using Symfony Form and Validator would be a real symfony bitching, also why don't we use symfony then).

I also think that the v1 Pagination class at least should be tear apart and have a pagination IO part and a renderer part. Then why don't use Pagerfanta for the IO part and write Renderers around it? Wait, Pagerfanta already supports renderers.

I am not defending that specific library. (Personally I think it is very well composed) I just think that the Pagination class I outlined above (and Pagerfanta itself) does exactly the same as v1 Pagination, but in a more sane and general way. If I should create a pagination package/class, I would create something very similar. Then why shouldn't use an existing one with the exact same functionality.

Regardless being third-party I think the functionality is perfect. Spending resources on getting to know the code and supporting it if something breaks is not only supporting the whole PHP community (anyone else using that library), but also takes less time than writting an own with the exact same functionality which will only be used by fuel users. (Maybe you could argue with the last argument, but I think that giving something to the community will eventually be returned)

emlynwest commented 9 years ago

I would much rather have our own pagination. As @WanWizard says it means we can better support it and better integrate it with other parts of fuel such as fieldsets and the ORM. One of the things I have been thinking of is passing a pagination object to ORM queries for automatic limit and offset functionality rather than needing to perform it manually.

sagikazarmark commented 9 years ago

I would say it is better to let the limiting part to the pagination through Adapters, like pagerfanta does (@stevewest did you check it?). Gives you more control over the pagination itself. But that's an architectural thing.

The argument of "we can better support it" is weird, because fuel will use league/event, whoops (for now). But I accept the decision.