-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Bug: in-model pager ignores the model event (with where clause, run via callback), reporting incorrect paging information #8412
Comments
You reset the query with echo 'total number of entries in table: ' . $model->countAllResults(); Try |
Tried; it changes nothing. Updated the test code above. And thanks for code colouring :) |
Ah, |
I assumed that, no problem here. It is the results at this line in output:
and later that are the problem. Pager counts the number of results that the model returns wrong. |
My theory of this error after looking in the code of The The method $this->pager = $pager->store($group, $page, $perPage, $this->countAllResults(false), $segment); How does that sound? |
It seems that we cannot solve this issue easily. When we generate pagination, we need to:
Also: If we call So the query order is not wrong in the current code: CodeIgniter4/system/BaseModel.php Lines 1228 to 1244 in 4308c0b
|
I don't know how to fix this issue or if we should not fix it. Workaround is to override public function paginate(?int $perPage = null, string $group = 'default', ?int $page = null, int $segment = 0)
{
// Fire restrictToUser() manually. // Added
$this->restrictToUser([]); // Added
// Since multiple models may use the Pager, the Pager must be shared.
$pager = Services::pager();
if ($segment !== 0) {
$pager->setSegment($segment, $group);
}
$page = $page >= 1 ? $page : $pager->getCurrentPage($group);
// Store it in the Pager library, so it can be paginated in the views.
$this->pager = $pager->store($group, $page, $perPage, $this->countAllResults(false), $segment);
$perPage = $this->pager->getPerPage($group);
$offset = ($pager->getCurrentPage($group) - 1) * $perPage;
// Disable model events for findAll(). // Added
$this->allowCallbacks(false); // Added
return $this->findAll($perPage, $offset);
} |
I see; I was looking at the code yesterday and also could not envisage a way to improve it (but thought maybe the more experienced people might). The suggested workaround would work I guess while I only have that single callback; either that or just add the needed where clause manually before the call to pagination (as I do now; it does generate a double WHERE but it works). I am leaving the issue open, but I am ok with it being closed as WONTFIX if an efficient solution cannot be found. Thanks for investigating, @kenjis! |
I wouldn't say it's a bug. Documentation is clear, what methods are affected by the database events: https://codeigniter.com/user_guide/models/model.html#event-parameters Although this may be inconvenient, just like in the presented case. To make it work, we would have to modify the |
According to the current documentation, it is not a bug. The behavior is expected. My question is: should |
The events only work on model-specific methods and countAllResults is from the query builder. I understand that this is a weird case but no I don't think so. The events were never intended to change the number of results, only modify the results that were found. Filtering should happen after results are returned. |
@lonnieezell Model has Line 611 in 4308c0b
Do you say the following Model callback is kind of a misuse? protected function restrictToUser(array $data)
{
// in my case the user_id comes from session, hardcode here:
$this->where($this->table . '.user_id', 3);
return $data;
} |
Ah, I forgot that was overridden for soft-deletes. Seems I misspoke. Would I say it's a misuse? No, just not what events were originally thought of. Everything always gets used in creative ways you didn't expect. Part of the fun - seeing how others use your stuff. :) Thinking of it another way, though, no I wouldn't expect a |
This is Controller function: <?php
namespace App\Controllers;
use App\Models\BusinessModel;
use CodeIgniter\RESTful\ResourceController;
use App\Models\{
BusinessProfilesModel,
ClientApplicationsModel,
};
use Config\{
Database,
Services,
};
class BusinessController extends ResourceController
{
protected $modelName = ClientApplicationsModel::class;
protected $format = 'json';
public function __construct()
{
// Load the model
$this->db = Database::connect(); // Forzar la conexión a la base de datos
$this->pager = Services::pager();
}
public function apps(){
$model = model(\App\Models\ClientApplicationsModel::class);
echo PHP_EOL;
echo 'Total number of entries in table (should be 99): ' . $model->countAllResults(false);
echo PHP_EOL . PHP_EOL;
$list = $model->findAll();
//$list = $model->where('user_id', 3)->findAll();
echo 'Number with user_id = 3, using findAll(), event restrictToUser applied (should be ~1/3 of 99): ' . count($list);
echo PHP_EOL . PHP_EOL;
echo 'Rerun find with paginate() (squeze all results in one page):' ;
$list = $model->paginate(101);
$pager = $model->pager;
echo '... done.' . PHP_EOL . PHP_EOL;
echo 'Real number of results, that honours event restrictToUser (should be ~1/3 of 99): ' . count($list);
echo PHP_EOL . PHP_EOL;
echo 'pager-reported number, that ignores event restrictToUser (should be ~1/3 of 99, but is 99): ' . $pager->getTotal();
echo PHP_EOL . PHP_EOL;
echo 'Rerun find with paginate() (10 per page):' ;
$list = $model->paginate(10);
$pager = $model->pager;
echo '... done.' . PHP_EOL . PHP_EOL;
echo 'Pager-reported number, that ignores event restrictToUser (should be ~1/3 of 99, but is 99): ' . $pager->getTotal();
echo PHP_EOL . PHP_EOL;
echo 'Pager-reported page number, that ignores event restrictToUser (should be ~3, but is 10) : ' . $pager->getPageCount();
echo PHP_EOL . PHP_EOL;
} Testing
|
To solve this error of deprecate documentation oficial of with bug on: /**
* Retrieves paginated businesses for a specific user using raw SQL.
*
* @param int $page The current page number
* @param int $limit The number of items per page
* @param string $userId The user ID to filter the businesses
* @return array Paginated business data and pagination details
*/
public function getPaginatedResults(int $page, int $limit, string $userId): array
{
// Calculate the offset
$offset = ($page - 1) * $limit;
// SQL query to get paginated results
$sql = "SELECT * FROM $this->table WHERE user_id = :user_id: ORDER BY created_at ASC LIMIT :limit: OFFSET :offset:";
// Execute the query
$query = $this->db->query($sql, [
'user_id' => $userId,
'limit' => $limit,
'offset' => $offset
]);
// Get the results
$results = $query->getResult();
// SQL query to count the total records
$countSql = "SELECT COUNT(*) as total FROM $this->table WHERE user_id = :user_id:";
$totalQuery = $this->db->query($countSql, ['user_id' => $userId]);
$total = $totalQuery->getRow()->total;
// Build the response with paginated data and pagination details
return [
'data' => $results,
'pagination' => [
'total' => $total,
'per_page' => $limit,
'current_page' => $page,
'last_page' => ceil($total / $limit),
'next' => $page < ceil($total / $limit) ? $page + 1 : null,
'previous' => $page > 1 ? $page - 1 : null,
],
];
} use to query please use warning with Validator to restrict access or sql insection to $userId; public function apps()
{
$pager = service('pager');
$page = intval($this->request->getVar('page', FILTER_VALIDATE_INT)) ?: 1;
$limit = intval($this->request->getVar('limit', FILTER_VALIDATE_INT)) ?: 10;
$userId = $this->request->getVar('user_id');
if (empty($userId)) {
return $this->fail('User ID is required.', 400);
}
if ($page <= 0) {
return $this->fail('Page number must be greater than 0.', 400);
}
if ($limit <= 0) {
return $this->fail('Limit must be greater than 0.', 400);
}
try {
$result = $this->model->getPaginatedResults($page, $limit, $userId);
return $this->respond([
'status' => 200,
'error' => null,
'data' => $result['data'],
'pagination' => $result['pagination'],
]);
} catch (\Exception $e) {
return $this->fail('Error in querying the database: ' . $e->getMessage(), 500);
}
} {
"status": 200,
"error": null,
"data": [
{
"client_id": "50512010-624f-11ef-b9bf-7071bcbe887a"
},
{
"client_id": "5c1a2230-6254-11ef-b9bf-7071bcbe887a"
},
{
"client_id": "620bd04f-6258-11ef-b9bf-7071bcbe887a"
},
{
"client_id": "57f55e69-625c-11ef-b9bf-7071bcbe887a"
}
],
"pagination": {
"total": "4",
"per_page": 10,
"current_page": 1,
"last_page": 1,
"next": null,
"previous": null
}
} |
PHP Version
8.1
CodeIgniter4 Version
4.4.4
CodeIgniter4 Installation Method
Composer (using
codeigniter4/appstarter
)Which operating systems have you tested for this bug?
Linux
Which server did you use?
cli-server (PHP built-in webserver)
Database
SQLite3
What happened?
I have a custom event in a model that applies where clause to every search implemented thus:
protected $beforeFind = ['restrictToUser'];
When I use it with an in-model pager, the find results apply this event, but the pager does not, resulting in wrong page count and wrong total record number.
Steps to Reproduce
Start with empty project.
Rename
env
to.env
, change these lines in it to the values:Add a cli route in app/Config/Routes.php:
$routes->cli('home', 'Home::index');
Add migration for a table:
php spark make:migration CreateTestTable
Replace up() and down() methods with:
make a seeder:
php spark make:seeder test --suffix
Replace the seeder method run() with this:
Create model:
php spark make:model TestModel
in model, update the $beforeFind callback thus:
protected $beforeFind = ['restrictToUser'];
and add event method to model:
Add test code to the Home controller, replace the index() method with:
Run the test :
Expected Output
I expect the in-model pager report the same number of entries as returned by the model paginate() method, applying the in-model specified event correctly.
Anything else?
No response
The text was updated successfully, but these errors were encountered: