fix: model event consistency in BaseModel::paginate()#9617
fix: model event consistency in BaseModel::paginate()#9617michalsn wants to merge 1 commit intocodeigniter4:developfrom
BaseModel::paginate()#9617Conversation
|
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 :) |
|
The issue I see with the proposed approach is that it introduces inconsistency. When using I considered introducing new events like The main problem is that we shouldn't trigger the events from 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. |
|
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. |
|
I've created an alternative PR that only adds a note to the user guide: #9618. |
|
We avoided trying to add conditions in |
Description
This PR fixes
BaseModel::paginate()to triggerbeforeFindevents before callingcountAllResults(), ensuring both the count and results are consistently filtered.Currently,
BaseModel::paginate()has an inconsistency wherebeforeFindevents 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:
beforeFindevents before counting to apply query modificationsfindAll()to prevent duplicate event executionafterFindevents to maintain a complete event flowI'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: