Making the pagination parameter configurable - RFC
DorsetDigital opened this issue · 6 comments
We've come across a few issues with pagination when using (in particular) Stackpath CDN. Sometimes, the presence of the 'start' parameter upsets the CDN (because.. reasons).
My immediate thought was that the pagination parameter should be configurable in the blog. The PaginatedList class has methods to support this, and the blog already reads the parameter name when doing its pagination. What I can't see is any method for setting the parameter in the blog system.
I found an open issue here silverstripe/silverstripe-framework#8306 regarding changing the pagination system altogether, but that issue is quite old, so I'm guessing there's no movement on it.
So, before I charge headlong into a PR, I wanted to gather some opinion:
Is it worth trying to do this as part of the blog module, or would it be better for me to look at a PR on PaginatedList to make the pagination parameters pretty instead and do away with the get var altogether?
Assuming it's worth pursuing here... my thinking was that I would make the parameter a config option on the BlogController, so it could be set via yml, etc. Then in BlogController::PaginatedList() call PaginatedList::setPaginationGetVar() to do the business.
would it be better for me to look at a PR on PaginatedList to make the pagination parameters pretty instead and do away with the get var altogether?
Something like this would have to be in a major release - if you want to be able to resolve the issues you've seen with blog pagination in v4, the blog pagination parameter will need to be made configurable.
my thinking was that I would make the parameter a config option on the BlogController, so it could be set via yml, etc. Then in BlogController::PaginatedList() call PaginatedList::setPaginationGetVar() to do the business
This seems like a reasonable approach - however before that I think it's worth asking if the start getvar is a problem for you with blog pagination specifically, or if it's a problem for other PaginatedLists as well (and you've just already configured those to use a different var)?
It seems to me it may make sense to add a yml configurable value on PaginatedList to set the default value, so that all new PaginatedLists across the site will have some other var.
That would of course require some regression testing to make sure nothing's expecting start explicitly.
I did wonder about that. The parameter name is already stored in a protected property on the class, so in theory I could just switch that to private and make sure all the references to it are replaced with the relevant config call. That shoudn't affect the public API, so hopefully it can be included in a minor release.
I'm happy for this to be closed if there's no other input. I can look at a PR on framework instead of here.
protected is part of the "public" API for non-final classes, because someone could have a subclass which relies on that property. And that protected variable is used for setting the value for a specific instance of PaginatedList rather than the global default which is what the new configuration variable would be for. So I'd leave the existing variable (but remove its default value) and add a new configuration variable - then in the constructor, call setGetVar(static::config()->get('default_get_var')); or similar.
But yes, if you're happy that add a global default configurable value in PaginatedList will resolve this then I agree to closing this issue.
For the blog module specifically, an updatePaginatedList() extension hook in BlogController::PaginatedList() might be enough?
It possibly would Loz, but I'm tending toward just sorting it out on a global level in PaginatedList. As Guy correctly points out, if this is an issue with blog pagination, then in theory, anywhere else which uses pagination could also be an issue.
If there's a use-case where specifically the blog module's pagination getvar is problematic, then yes that would be a good solution.