CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

`Model::paginate()` behavior when `$page` exceeds the last page

Open crustamet opened this issue 1 year ago • 21 comments

PHP Version

8.3

CodeIgniter4 Version

4.1.5

CodeIgniter4 Installation Method

Manual (zip or tar.gz)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

MySQL 5.6

What happened?

When i paginate the model like this $UsersMode->orderBy('id_user', 'ASC')->paginate(8, 'group', $page_number);

returns "SELECT * FROM users ORDER BY id_user ASC LIMIT 8, 8" based on offset and per page number

But my total count rows in the table are 24, so when i reach the end 16, 8 it returns me the same query as the last existing page in my database even with $page_number = 100

Steps to Reproduce

	public function load_more($page_number = 2)
	{
		$UsersModel 	= new UsersModel();
		
		$this->ViewData['Page'] = $page_number;
		
		$this->ViewData['Users'] = $UsersMode->orderBy('id_user', 'ASC')->paginate(8, 'group', $page_number);

		echo $UsersModel->db->getLastQuery();
	}

returns "SELECT * FROM users ORDER BY id_user ASC LIMIT 8, 8"

But my total count rows in the table are 24, so when i reach the end 16, 8 $page_number = 10 and go to a page number more than 3 it returns the same query "SELECT * FROM users ORDER BY id_user ASC LIMIT 16, 8"

Expected Output

Going to a page number more than the total count rows of my table it should return the propper offset even if i have the total rows less than the per page number or less than the total count

$page_number = 10 it should returns the query "SELECT * FROM users ORDER BY id_user ASC LIMIT 80, 8" even if i don't have enough records in my database table

Anything else?

No response

crustamet avatar May 19 '24 05:05 crustamet

one this BaseModel https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/BaseModel.php#L1273

	public function paginate(?int $perPage = null, string $group = 'default', ?int $page = null, int $segment = 0)
	{
		// Since multiple models may use the Pager, the Pager must be shared.
		$pager = service('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;

		return $this->findAll($perPage, $offset);
	}

should be converted to something like this

	public function paginate(?int $perPage = null, string $group = 'default', ?int $page = null, int $segment = 0)
	{
		// Since multiple models may use the Pager, the Pager must be shared.
		$pager = service('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);
		$offset      = ($page) * $perPage;
		$perPage     = $this->pager->getPerPage($group);

		return $this->findAll($perPage, $offset);
	}

crustamet avatar May 19 '24 06:05 crustamet

@crustamet I don't understand the difference. Please show the diff by diff command or git diff.

kenjis avatar May 19 '24 07:05 kenjis

$page_number = 10 it should returns the query "SELECT * FROM users ORDER BY id_user ASC LIMIT 80, 8" even if i don't have enough records in my database table

Why?

kenjis avatar May 19 '24 07:05 kenjis

it doesn't matter the difference because how i did it is no good it is just to point out that if the total records in the database are 10

when you go to page 1000

the query remains as of the last page so the limit whould be {a long number}, {8}

I dont have pagination only a load more button

"Why?" - because i cannot remove the button when i have no records, it always gives me records even when i don't have so it always shows load more

crustamet avatar May 19 '24 08:05 crustamet

$page_number = 10 it should returns the query "SELECT * FROM users ORDER BY id_user ASC LIMIT 80, 8" even if i don't have enough records in my database table

Why?

First of all it should return that query if you use

$Model->paginate(8, 'group', 10); it should create the query above ? right or wrong ?

While the current implemented code is dependent on how many rows you actually have in the table

If you have 10 users and you are at page 10

$Model->paginate(8, 'group', 10); This returns "SELECT * FROM users ORDER BY id_user ASC LIMIT 16, 8"

I cannot return a 404 page if i have no more users on page 10 LOL

crustamet avatar May 20 '24 05:05 crustamet

It seems this behavior is intentional. https://github.com/codeigniter4/CodeIgniter4/blob/8fc89854cc85c3b002ce2b26d5de56f6fa131166/tests/system/Models/PaginateModelTest.php#L78-L85

kenjis avatar May 20 '24 08:05 kenjis

You can check if it has any more pages with $pager->hasMore().

kenjis avatar May 20 '24 08:05 kenjis

Ok i get what you are saying, I dig this $pager->hasMore() method it does what I need a kind of a checker, but i have millions in my database, i just can't wrap my head around this one why it is an intentional behavior. while the query speed is slower when you select from millions at the last page than if you select from millions from a page that does not exists. Furthermore you don't actually need to add additional code to check this as pagination model will return an empty array if you go out of range

Anyways up to you guys, $pager->hasMore() is missing from the documentation

crustamet avatar May 20 '24 11:05 crustamet

I don't know why the pagination behaves like that. But test code is document that describes how the system works, so as you see the test code, the current behavior is intentional.

If we change the behavior, it will be an breaking change.

If you insist that this behavior is something that needs to be corrected, please send a PR to 4.6 branch and explain reasons that many people would agree. If it is to meet the needs of just one of you, you can simply customize the pagination.

In my opinion, this pagination was not originally designed to handle very large amounts of data at high speed. If you want to process large amounts of data at high speed, I recommend a different implementation without using OFFSET.

kenjis avatar May 20 '24 13:05 kenjis

I agree with everything you said, actually i will extend the current BaseModel paginate() maybe in time i will understand why it returns data even if it is out of range.

I hope some other guy read this and maybe help us understand better

crustamet avatar May 20 '24 13:05 crustamet

I've known about this behavior for a long time. At first, this caused a misunderstanding of "Why?" Such a feature, I think, was popular in 2009. This is not critical at this stage. Problems may appear if you use paginate() for the API (strict result is important). As a fix, it is possible to create a new strictPaginate() method and remove this behavior

neznaika0 avatar Jun 06 '24 16:06 neznaika0

The method name pagenateStrict() is better, but I agree with adding the new method.

kenjis avatar Jun 07 '24 00:06 kenjis

i made it and tested it The old one

    public function paginate(?int $perPage = null, string $group = 'default', ?int $page = null, int $segment = 0)
    {
        // Since multiple models may use the Pager, the Pager must be shared.
        $pager = Services::pager();

        if ($segment) {
            $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;

        return $this->findAll($perPage, $offset);
    }

The new one

	public function paginateStrict(?int $perPage = null, string $group = 'default', ?int $page = null, int $segment = 0)
	{
		$pager = Services::pager();
		
		if ($segment) {
			$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      = ($page - 1) * $perPage;
	
		return $this->findAll($perPage, $offset);
	}

crustamet avatar Jun 28 '24 08:06 crustamet

I've looked at the code.

In a good way, you need to change Pager->store(), $page is illogically set in it, $page has a "raw" value in line 148, and then it in line 154 is changed to limit. https://github.com/codeigniter4/CodeIgniter4/blob/4151c126ceb772d80d496d7207d3027e050a0683/system/Pager/Pager.php#L147-L154

Similar actions in BaseModel->paginate(): On line 1282 it has one value, then it is not used for $offset https://github.com/codeigniter4/CodeIgniter4/blob/4151c126ceb772d80d496d7207d3027e050a0683/system/BaseModel.php#L1282-L1286

So the main reason is to change the currentPage. Will we find more options instead of editing the BaseModel?

neznaika0 avatar Jul 02 '24 17:07 neznaika0

I think this feature was implemented in the Feature.php

/**
 * The behavior of `limit(0)` in Query Builder.
 *
 * If true, `limit(0)` returns all records. (the behavior of 4.4.x or before in version 4.x.)
 * If false, `limit(0)` returns no records. (the behavior of 3.1.9 or later in version 3.x.)
 */
public bool $limitZeroAsAll = false;

Right ? this issue should be closed.

crustamet avatar Jan 10 '25 09:01 crustamet

No. limit(0) ‐ switches receiving 1000/1000 rows or 0/1000 rows

neznaika0 avatar Jan 10 '25 09:01 neznaika0

So with with this

public bool $limitZeroAsAll = false;

If I have a total of 10 Pages If I go to the Page 11, will get records ? or not ? :)))

If you still get records for sure this is not implemented... and in fact this limitZeroAsAll has nothing to do with pagination

crustamet avatar Jan 10 '25 09:01 crustamet

https://github.com/codeigniter4/CodeIgniter4/issues/8278

neznaika0 avatar Jan 10 '25 10:01 neznaika0

i don't think you got my issue, This is another issue https://github.com/codeigniter4/CodeIgniter4/issues/8278

Please answer my question as the answer matters the most

If I have a total of 10 Pages If I go to the Page 11, will get records ? or not ?

And it has nothing to do with ->limit(0,0);

I've tested it the answer is still YES they are still returning data even tough you are exceeding the total pages

If i go to the Page 11, but I only have records for 10 pages it should return NO RECORDS !

Why ? because of SEO they see duplicate pages and someone can create issues by just adding +1 to the page number

Page 11, Page 12, Page 10000 they will be all issues ! of duplicate content

crustamet avatar Jan 10 '25 10:01 crustamet

I understood the problem, but you don't understand the $limitZeroAsAll parameter. That's why I pointed to the issue. The answer to your question is: it doesn't affect Pager.

You can extend the Pager class and overwrite the service. All. This is the fastest solution for you right now. https://codeigniter4.github.io/userguide/extending/core_classes.html#replacing-core-classes

neznaika0 avatar Jan 10 '25 10:01 neznaika0

I know @neznaika0 i did extended the pager for sure as i did not have any other solution.

that parameter $limitZeroAsAll should be related with the Pager from my point of view...

crustamet avatar Jan 10 '25 10:01 crustamet