codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.39k stars 1.9k forks source link

Bug: Model->setTable does not change table #5207

Closed Valkhan closed 3 years ago

Valkhan commented 3 years ago

Describe the bug I need a generic model to perform simple queries without having to load an especific model

CodeIgniter 4 version 4.1.14 (latest)

Affected module(s) Model/query builder

Expected behavior, and steps to reproduce if appropriate Just try to perform a query in different tables using setTable

query 1: setTable(users) - OK query 2: setTable(roles) - Fail, it will use the users table to construct the query

Exemple code


$dModel = new \CodeIgniter\Model();
var_dump($dModel->setTable('users')->getCompiledSelect());
// OUTPUT: string(21) "SELECT * FROM `users`" 
echo '<br>';
var_dump($dModel->setTable('users_preferences')->getCompiledSelect());
// OUTPUT: string(21) "SELECT * FROM `users`" 
// SHOULD OUTPUT: string(21) "SELECT * FROM `users_preferences`" 
kenjis commented 3 years ago

I don't know this is a bug or not. Because theCodeIgniter\Model is supposed to be used by inheritance. https://codeigniter4.github.io/CodeIgniter4/models/model.html#creating-your-model

By the way, could you show us the use case of yours? The sample code just run getCompiledSelect(). It is probably not what you really do.

And why do you want to use CodeIgniter\Model for simple queries?

lonnieezell commented 3 years ago

It's not really a bug. It caches the builder instance it creates using the table. For what you want, don't use setTable, use builder($tableName) instead. This is typically used from within the model.

I will say, though, that CodeIgniter models are designed to be used with a single table so you're bound to hit issues. Using the Query Builder by itself is the best way to go for what you're trying. Which is as simple as doing db_connect()->table('users_preferences')->where('user_id', $userId)->get();

Valkhan commented 3 years ago

Thanks @lonnieezell your solution on using ->builder($tableName) is the way to go.

Sorry for the delayed response, but I wanted to do some tests and explain in details my use case.

I do think that if someone needs/wants to change table by using ->setTable($tableName) and the framework by itself caches it's builder instance, it should also purge the cache and reset the builder inner table reference as well, since "setTable" is the most semantically correct alternative, and the first option in the docs when someone is working with Models, I would suggest to treat this issue not as a Bug but perhaps and enchancement.

@kenjis as for use cases, there are many, but the most commom and usefull for this dynamic use of a single Model for many tables are:

1st faster development, for small tables with just Key pair values like parameters, user preferences, configurations.... and also for tables that are readonly or data is populated elsewhere without user interaction, the whole proccess of creating a file and ajusting it to use Models cost too many time for almost no gain, having a simple query done without an specific Model attached we can spare some effort and some lines of code, with @lonnieezell solution, I've spare 4 model instances (4 variables that would be consuming memory) and 8 lines of code in a single function, if you need to repeat this proccess timed again... that will lead to time comnsuming and boring repetitive tasks.

2nd reusable code and dynamic models, for so many years I've developed a data dictionary that works somewhat like Wordpress Form Builder plugin, that means that my database holds everything that is necessary to a Model to work properly, and I'm migrating it to CI4 from CI3, the hability to seek data in my data dictionary and other readonly tables meant for a better quality code helps me mitigate some "developer mistakes" so having the means to create and modify a Model by runtime without the need of a written file is a very positive in my perspective since we can create a CRUD without the need of a developer and we act just on what's important: client specific demands and business logic, that particular example is very important in an economic point of view.

3rd Performance, I haven't tested it yet so correct me if I'm wrong, but I suppose creating a new instance of a Model costs more server memory, and maybe opens more database connections, on CI3 the model reusage was clearer than in CI4, and normally my app would open at most 8-16 connections even with 5k simultaneos users, I don't know if in this scale, having a routine that access multiple tables each in its own instance it would cost more server resources and thus money.

Real and recent use case, but with Codeigniter 3:

I recently developed a promotional campaign for iFood where the simple fact of loading the database without using queries already exceeded the limit of simultaneous connections with the database when their marketing campaing hitted TV or instagram, which in an immediate action increased our cloud server costs, we did the adjustment to only open connection with the database at the time of user registration and that fix alone saved us about 80% in costs with infrastructure.

I would be very glad to hear from you guys if my concerns about new Model instantiations would open new connections to database and understant if my worries about performance are real or am I just too paranoid :)

Thank you all for the awesome job, effort and dedication to this project, I'm glad to give this detailed feedback.

lonnieezell commented 3 years ago

That's a good point about setTable() and would definitely qualify as an enhancement. That would probably have to wait until 5.0 since it's a breaking change.

If you use model() to load your models you will only ever have a single instance of each model, so no worry about the memory usage there.