Bug: Invalid page number change in 9+

[phyzical]

👀 Before submitting...

• [X] I upgraded to pagy version 9.0.9
• [X] I searched through the Documentation
• [X] I searched through the Issues
• [X] I searched through the Q&A

🧐 REQUIREMENTS

• [X] I am providing a VALID code file that confirms the bug
• [X] I am NOT posting any of the USELESS THINGS listed above
• [X] I am aware that this issue will be automatically closed if the code file is missing or INVALID

💬 Description

Not sure if this is a bug but in 9 we had a set of tests fail that assumed that invalid page numbers result in the first page to be returned i.e providing page: -1 page: 'blah'

in 8 would result in the first page of results in 9 we get

Pagy::VariableError:
       expected :page >= 1; got -1

Pagy::VariableError:
       expected :page >= 1; got 0

TBH, i think its just a set of silly tests ourside but i could see it biting larger projects where people do dodge to reset pages ect.

Might just be worth getting added to the breaking changes list?

[phyzical]

incase others need this and this is closed as 'not fixing' i monkey patched it by

@vars[name] = 1 if name == :page && (@vars[name].instance_of?(String) || (@vars[name]&.respond_to?(:to_i) && @vars[name].to_i <= 0))

class Pagy
  module SharedMethods
 def assign_and_check(name_min)
      name_min.each do |name, min|
        @vars[name] = 1 if name == :page && (@vars[name].instance_of?(String) || (@vars[name]&.respond_to?(:to_i) && @vars[name].to_i <= 0))
        raise VariableError.new(self, name, ">= #{min}", @vars[name]) \
        unless @vars[name]&.respond_to?(:to_i) && \
               instance_variable_set(:"@#{name}", @vars[name].to_i) >= min
      end
    end
  end
end

[ddnexus]

@phyzical thank you for your report.

I am wondering how you don't get the automatic assignation of the page variable, through the pagy_get_page method.

Could you please use one of the Playground apps to show how to make it fail?

[phyzical]

I assume then it is just simply the fact its a redundant spec causing the issue to begin with, as its explicitly passing in the param.

Based on what your saying its probably safe to just close this. Just figured id raise it as it does seem to be a functionality change not mentioned in the breaking changes.

context 'when the client passes a nonsense page param' do
    it 'renders the first page instead of falling over' do
      login_as_user users(:global_admin)
      get users_path(page: -1)
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')

      get users_path(page: 'nonsense')
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')

      get users_path(page: '')
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')
    end
  end

[phyzical]

and why it happens /lib/pagy/backend.rb

vars[:page] ||= pagy_get_page(vars) its just early exiting as its explicitly provided

[ddnexus]

That explains it. Indeed, if you explicitly pass a variable, you are responsible for passing it right.

Just figured id raise it as it does seem to be a functionality change not mentioned in the breaking changes.

I will check whether the docs and breaking changes are consistent with the current behavior before closing it.