Harden the Paginate constructor against missing/invalid config - #213
Open
pcbeingused333 wants to merge 2 commits into
Open
pcbeingused333 wants to merge 2 commits into
pcbeingused333 wants to merge 2 commits into
Conversation
`base_path` is an optional keyword argument, but when it is omitted (or passed as `nil`) and the collection spans more than one page, `next` and `previous` build the path with `@base_path.sub(...)` and raise `NoMethodError: undefined method 'sub' for nil`. Default it to `"/"` (and coerce an explicit `nil`), so an omitted `base_path` yields root-relative paths like `/page/2/` instead of crashing. Callers that pass a `base_path` are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GtYgwYNfJ3wHrw9xrooVoB
Author
|
CI is on |
`config.pagination.per_page` has no default, so an unconfigured collection reaches `Paginate.new(per_page: nil)` and dies with `TypeError: nil can't be coerced into Float`. `per_page: 0` gives `FloatDomainError: Infinity`, and a negative value gives `ArgumentError: min argument must be smaller than max argument` from the `clamp`. Validate up front with a message that says what is wrong. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GtYgwYNfJ3wHrw9xrooVoB
Paginate#base_path to "/" instead of nilPaginate constructor against missing/invalid config
Author
|
Updated: added |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two ways
Perron::Paginate.newcrashes on config that a caller can realistically hand it.1.
base_pathomitted →NoMethodErrorbase_path:is an optional keyword (base_path: nil), but omitting it breaksnext/previousonce the collection has more than one page:previousfrom page 2 also returnsnileven thoughprevious?istrue. Fixed by defaultingbase_pathto"/"(and coercing an explicitnil), so an omittedbase_pathyields root-relative paths (/page/2/,/).2. non-positive
per_page→ cryptic errorsconfig.pagination.per_pagehas no default (onlypath_templateis set inResource::Configuration), so an unconfigured collection reachesPaginate.new(per_page: nil):per_pagenilTypeError: nil can't be coerced into Float0FloatDomainError: InfinityArgumentError: min argument must be smaller than max argument(fromclamp)Now validated up front:
ArgumentError: per_page must be a positive integer, got nil.Tests
Four new cases in
paginate_test.rb(next/previouswithbase_pathomitted and explicitlynil;per_pageof0/-5/nil/2.5). All fail onmain.bundle exec rakeis green (405 runs, 0 failures;standardrbclean).🤖 Generated with Claude Code