Skip to content

fix: model event consistency in BaseModel::paginate() #9617

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

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

Conversation

michalsn
Copy link
Member

Description
This PR fixes BaseModel::paginate() to trigger beforeFind events before calling countAllResults(), ensuring both the count and results are consistently filtered.

Currently, BaseModel::paginate() has an inconsistency where beforeFind events are only applied to the data retrieval query (findAll()) but not to the count query (countAllResults()). This causes pagination metadata to be incorrect when model events filter or modify the underlying query. More details and examples are available in #8412.

The solution in short:

  • We use a temporary pager instance to determine the actual pagination parameters
  • Trigger beforeFind events before counting to apply query modifications
  • Disable callbacks during findAll() to prevent duplicate event execution
  • Then manually trigger afterFind events to maintain a complete event flow

I'm not 100% certain this is the proper way to solve this issue. While it fixes the consistency problem, it does change the execution flow a bit. If anyone sees any drawbacks, please share.

Also, I feel like the changes are a bit messy, so feedback is much appreciated on both the technical implementation and whether this change should be made at all.

Fixes #8412

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn michalsn added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 28, 2025
@mavelo-llc
Copy link

Considering the problem, it may look/feel messy, but it saves some of us from having to do something similar on each project to achieve accurate results. Thanks for jumping on this :)

@michalsn
Copy link
Member Author

The issue I see with the proposed approach is that it introduces inconsistency. When using paginate(), the countAllResults() method benefits from the beforeFind event. However, when calling countAllResults() directly, it does not. That said, the current behavior already doesn't align well with users' expectations either 😅

I considered introducing new events like beforeCount and afterCount, which might help clarify the behavior. However, I'm not entirely convinced - that could introduce additional confusion. More importantly, making this consistent with other methods would require changes to how the model works.

The main problem is that we shouldn't trigger the events from Model, but rather from BaseModel, and doing so would result in breaking changes.

I have to admit, I'm a bit stuck. This approach makes more sense than the alternatives, but I still find it hard to decide.

@neznaika0
Copy link
Contributor

I use model events only to modify the result - updating entities (change some values or add many-to-one dependencies). So the request is the same for the Pager.

It may be worth adding a warning in the documentation - that there is an inconsistency if you modify the query during pagination.

@michalsn
Copy link
Member Author

michalsn commented Jul 1, 2025

I've created an alternative PR that only adds a note to the user guide: #9618.
Until we develop a common position on the best way out of this situation, I think this note is probably the best path forward for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: in-model pager ignores the model event (with where clause, run via callback), reporting incorrect paging information
3 participants