search

Protoqol / Prequel

Prequel for Laravel. Clear and concise database management. Unfortunately, not actively maintained due to time constraints.
https://packagist.org/packages/protoqol/prequel
MIT License
1.54k stars 96 forks source link

[BUG] Counting of records will fail with a non-id column #27

Closed aaronsaray closed 5 years ago

aaronsaray commented 5 years ago

Describe the bug First of all, I apologize if this is not really a bug. I did come across some code, though, that I wanted to help with. I didn't see exactly how it was used at first glance.

The issue is in DatabaseController::countTableRecords() where it executes a ->count('id') call. This can be a problem with tables that don't have a column named id. I've seen a table named cars which had a primary key of car_id instead of just plain id.

Some people will try to do count on a column that is a primary key like that because of performance gains. However, with covering indexes, that's not really something you need to force. Check out this article: https://www.percona.com/blog/2007/04/10/count-vs-countcol/ - while there are very RARE cases when it makes sense to count on a column, most Laravel apps will probably have an id field that is a primary key/or/covering index. (Well aaron - if that's the case, what's wrong with the code as is? Right now - it forces you to have that column. By using the default call to count() which defaults to column * we can handle the few situations where there is no id column.)

Suggested fix Modify count('id') to count('*')

QuintenJustus commented 5 years ago

You’re absolutely right! However, I don’t think the method is actually called anywhere as of yet. Optimisations are certainly welcome, but the method is there for a feature that will be implemented in the future and because of that has not been tested yet.

aaronsaray commented 5 years ago

Alrighty - thanks for the clarification. If you want to super-power your project with contributors, when you pre-write (is that even a word?) code, you might want to use the @todo annotation in there so people know that this isn't ready yet. You could say @todo complete this with testing - right now it's just a placeholder - that way people won't think its done+done :) I'm going to close the issue then because it's not a legit issue if it's not in use.