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

current_per_page always reports default page size for PaginatableArray #960

Closed
stevegeek opened this issue Aug 8, 2018 · 4 comments
Closed

Comments

@stevegeek
Copy link

on Kaminari core 1.1.1

It seems like if one creates a PaginateableArray using Kaminari.paginate_array then calls per() on the PaginateableArray the result of current_per_page is always the default not the value set in per.
eg

p = Kaminari.paginate_array(...).page(1).per(3)
p.current_per_page # 25

On looking into it I think this is cause

self.class.new @_original_array, limit: num, offset: @_offset_value, total_count: @_total_count, padding: @_padding

is called by the call to per() , which in turn creates a new instance of the PaginateableArray but that only sets the limit on @_limit_value , not @_per

https://github.com/kaminari/kaminari/blob/master/kaminari-core/lib/kaminari/models/array_extension.rb#L18

However current_per_page returns @_per or in the case its nil (which it is on the instances returned by limit/offset) the default value

Not sure if maybe the fix is to change PaginateableArray initializer to also set @_per to limit ?

@yuki24
Copy link
Member

yuki24 commented Aug 8, 2018

Thanks for reporting. I wonder if we really need the @_per instance variable. The use of instance variables is causing other issues like #955 as well, so it would be nice if we could just drop it and rely on the #limit_value interface (and require the collection object to confirm it).

@yuki24
Copy link
Member

yuki24 commented Aug 8, 2018

Actually, #875 is a PR that I merged that I shouldn't have merged. There's already #limit_value that does it and it doesn't make sense to add a new API that does the same. I'm going to deprecate #current_per_page.

@stevegeek
Copy link
Author

@yuki24 Thanks!

@yuki24
Copy link
Member

yuki24 commented Sep 4, 2018

closed by 7304650

@yuki24 yuki24 closed this as completed Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants