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

Fix offset not in array if on last page and filter is applied. #8829

Open
wants to merge 1 commit into
base: release_10
Choose a base branch
from

Conversation

thojou
Copy link
Contributor

@thojou thojou commented Jan 10, 2025

Hey,

This PR addresses the issue described in Mantis Bug Tracker https://mantis.ilias.de/view.php?id=42959.

During debugging, it became clear that the problem was not limited to a specific table implementation, but affected all table implementations. Here’s a scenario to illustrate the problem:

If a table has multiple pages and the user navigates to the last page, applying a filter that reduces the total number of displayed pages by at least one will trigger the issue.
The root cause lies in how the data table range is cropped—this happens too late in the execution flow. To address this issue, the PR introduces two adjustments:

  1. The findCurrentPage method now defaults to 0 instead of throwing a LogicException if no valid range is defined for the given offset.
  2. Previously, the croppedTo method returned a zero-length range, which seemed counterintuitive. This is adjusted so that the method now returns to the interval of 0 to smaller user selection or standard configuration.

These changes are directed to resolve the data tables as a whole.
Looking forward to your feedback and comments.

Best
@thojou

@thojou thojou added bugfix php Pull requests that update Php code labels Jan 10, 2025
Copy link
Member

@klees klees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @thojou,

thanks for looking into this. I'm not really convinced that this the way to move forward.

Regarding the change in Data\Range: I understand that you feel the current behaviour is counterintuitive. But the current behaviour is the current behaviour, so changing it just like that could lead to all kinds of surprises for other people. The idea is that the method limits the range to the upper bound given as $max, while being as faithful to the lower bound as possible. We're already in some edge case there, where we cannot keep the lower bound as is. But moving the lower bound to 0 while keeping the length just feels arbitrary in the light of the regular behaviour of the method. The method might be documented better, I guess, but the behaviour seems consistent to me. Why would we need that change?

Regarding the change in Renderer::findCurrentPage: Defaulting to some page if we are out of range of available pages seems to be fine. Why would we jump to the first page (instead of the last...)? No strong oppinion here, just an honest question if there are any arguments to be repeated sometimes 🙂

Kind regards
@klees (in agreement with @thibsy and @Amstutz)

Please assign back to me when I can move on with this.

@klees klees assigned thojou and unassigned klees Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix kitchen sink php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants