Bug: Undocumented backwards-incompatible change of empty "page" parameter introduced in 6.4.0

[clemens]

👀 Before submitting...

• I upgraded to pagy version 8.2.0
• I searched through the Documentation
• I searched through the Issues
• I searched through the Q&A

🧐 REQUIREMENTS

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

💬 Description

In an app I'm working on, I discovered a behavior that I hadn't seen before.

• Before: Calling an endpoint with page=&per_page= (empty values for both) would result in defaults being used
• After: Calling the endpoint fails with Pagy::VariableError: expected :page >= 1; got 0

How to reproduce with the Pagy playground app:

• No code changes needed
• Just navigate to http://localhost:8000/?page= – you see the error straight away. (I couldn't get it to run on 6.3.0, since it didn't yet have the pagy binary. But running my app with 6.3.0 works and 6.4.0 breaks it and it also breaks for 8.2.0.)

While researching, it looks like this commit introduced the change:

• Previously, the code to determine the page was params[vars[:page_param] || DEFAULT[:page_param]] – this would allow blank strings to be passed through to Pagy, where normalize_vars would remove it and thus effectively fall back to the default – in the case of page, that default would be 1
• Since the code now converts everything .to_i and "" (empty string) is is a truthy value, we get "".to_i, resulting in 0 – which, obviously, is neither nil nor a blank value and thus doesn't get cleaned up by normalize_vars anymore.
• Since the behavior in normalize_vars is apparently intended to clean up blank inputs, it looks to me like this is an unintended side effect.

Happy to provide a PR if desirable.

[ddnexus]

@clemens Thank you for the report and the complete description of bug and cause!

I remember there was a reason for the .to_i, but I don't remember which one ATM. It was something related to multiple overriding or sharing of the same method or something... I will investigate.

I will have to add a test, because right now nothing fails for the bug (not sure whether I also messed up the test for it).

Happy to provide a PR if desirable.

Thank you, that's always desiderable. However in this particular case I need some generalized deep thinking before I can fully understand the implications and fix it without breaking something else somewhere, somehow, sometime 🤔

[ddnexus]

8.2.1

[ddnexus]

@clemens FYI

The page variable is normally extracted from the params and sent to the Pagy.new, which can do its thing with normalization. However, certain extras need the page vars as a positive integer BEFORE creating the pagy object, so in that case the normalization comes too late.

The pagy_get_page takes care of the normalization of the page in all cases now, before creating the pagy object.