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

Model's without primary keys get pagination counts wrong #1597

Closed
lonnieezell opened this issue Dec 10, 2018 · 4 comments
Closed

Model's without primary keys get pagination counts wrong #1597

lonnieezell opened this issue Dec 10, 2018 · 4 comments
Labels
database Issues or pull requests that affect the database layer

Comments

@lonnieezell
Copy link
Member

It seems that I encountered another issue because of not using primary key. It seems that the new CodeIgniter's design is built around primary key system?

I supposed to have '5' results from my query.
I want to count how many result for pagination so I used $query->countAllResults() to count it, the result was '2', I don't know where '2' came from.
I then tried to view the result using $query->getResult(), it shows '5' results correctly. $query->paginate() shows correct result as well.

Here's the test code I used

$query = $this->select('name, SUM(ctr_choose) as total_choose');
$query->join('food', 'user_answer.id_food=food.id');
$query->groupBy('id_food');
$compiled = $query->getCompiledSelect();
print_r($compiled);
echo '|';
$this->resetQuery();
$query = $this->select('name, SUM(ctr_choose) as total_choose');
$query->join('food', 'user_answer.id_food=food.id');
$query->groupBy('id_food');
print_r($query->countAllResults()); //result: 2 (incorrect)

echo '|';
$query = $this->query($compiled);
print_r(count($query->getResult())); //result: 5 (correct)
	
$link = mysqli_connect("localhost", "root", "", "battermod");
$result = mysqli_query($link, $compiled);
$num_rows = mysqli_num_rows($result);
echo "\n <$num_rows Rows>\n"; //result: 5 (correct)

Because there's no $this->db->last_query();, I don't know what kind of query countAllResults produced.

Originally posted by @cobafesbuk in #1583 (comment)

@cobafesbuk
Copy link

Hi, I just noticed $query->get()->resultID->num_rows shows correct result.
Better workaround than count($query->getResult()).

@jim-parry jim-parry added the database Issues or pull requests that affect the database layer label Dec 10, 2018
@jim-parry jim-parry added this to the 4.0.0-beta.2 milestone Mar 5, 2019
@atishhamte
Copy link
Contributor

There is no point of keeping the statement,

$sql = ($this->QBDistinct === true) ? $this->countString . $this->db->protectIdentifiers('numrows') . "\nFROM (\n" . $this->compileSelect() . "\n) CI_count_all_results" : $this->compileSelect($this->countString . $this->db->protectIdentifiers('numrows'));

This should work with,
$sql = $this->compileSelect($this->countString . $this->db->protectIdentifiers('numrows'));

@lonnieezell
Copy link
Member Author

@atishamte Not sure where you're referring to with that query.

However, the more I think about this the less I'm concerned with having it work without a primary key. As we've added a bunch of new features to the model, many do rely on having a primary key to work. Many people also want an ORM and if that's ever going to happen, it's likely that we'll need to have primary keys on models.

So - closing this because we won't support that in models. I'll have it throw an exception when a model doesn't have a primary key so it's obvious.

@atishhamte
Copy link
Contributor

I was referring to the countAllResult function body

lonnieezell added a commit that referenced this issue Mar 14, 2019
…from becoming a tangled mess, and partially to ensure all Model convenience features work both now and in the future. Closes #1597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer
Projects
None yet
Development

No branches or pull requests

4 participants