Builder Tables

[MGatner]

Description

See #3793 for the underlying issue

This one might be controversial, but there's a bug and I have not found a simpler fix. Model::builder($tableName) will return an existing shared instance of the Builder regardless of $tableName's value. This is for a couple reasons:

• builder() is protected and the __call() method that loads it in turn does not pass on parameters
• If builder() detects a existing shared instance it will return it immediately, regardless of the table name

This PR attempts to fix the issue by:

1. Defining a $tableName property for Builder, assigned during initialization
2. Providing a getter to access this property
3. Adjusting Model::builder() to be public (also cleans up some unnecessary __call() steps)
4. Using Builder::getTable() to check the table against a requested name in the parameter

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I encountered this issue recently and never thought it was a bug. I was working with multiple DB tables in the same model class and thought that maybe Models were originally conceived to be one DB table per Model class.

My solution for this is rather simple but meant for one-time consumption only.

   /**
     * Always ensure the model is using the right table when
     * model uses multiple tables.
     *
     * @param string $table
     *
     * @return \CodeIgniter\Database\BaseBuilder
     */
    protected function ensureBuilderUsesTable(string $table)
    {
        $this->builder = $this->db->table($table);

        return $this->builder;
    }

Then it is used like this:

$this->ensureBuilderUsesTable('users');
$this->insert($data);

return $this->ensureBuilderUsesTable('logins')->delete('id', $id);

[MGatner]

Elegant! I don't think it solves the bug but it is a good workaround. I do think a model should be reference a single table only, and honestly the functionality of calling builder($tableName) externally is weird and probably should never have been there as $model->db->table($tableName) accomplishes precisely the same thing much cleaner. But since it is there I feel like it probably needs to stay? Maybe not? We don't reference the parameter in the User Guide.

[MGatner]

Removing testGetTableRespectsFrom which was from an earlier iteration of the code based on $QBFrom.

[paulbalandan]

If use of $this->builder() is causing more confusion than benefits, then I would be more inclined of deprecating it in favor of the more direct $this->db->table(). But I think it will entail a lot of rework since is used already throughout the Model.

[MGatner]

I think what I would (ideally) like to see is builder() made private so it could still be called by Model but not by child classes, then reinstate its availability to __call() which inherently does not take a parameter so circumvents this whole issue. The problem is that because builder() is currently protected, there are possibly people using it - for example, me :) https://github.com/tattersoftware/codeigniter4-permits/releases/tag/v2.1.4 which is how I found the bug to begin with.

[MGatner]

I suppose an alternate solution that might head more in the deprecation direction would be:

1. Only store the shared builder instance when $tableName === $this->table
2. In __call($name, $params) if $name === 'builder' and ! empty($params) we throw an exception for "builder() cannot accept external parameters"

[lonnieezell]

I like the implementation. It makes it work like I thought it always worked lol. Pretty sure I'd used it like that before. But the intent was always $this->builder matches for the current table. But you were always free to call $this->builder($table) to get an instance to another table.

[MGatner]

Good to know @lonnieezell! I like that this cleans things up a bit beyond the bug itself; I don't like the table tracking on BaseBuilder but because all other table references are prone to escaping & prefixing they aren't safe for comparison agains Model::$table. Can't win them all though.

[michalsn]

I like these changes.

Also, I'm not a fan of the idea of depreciating the builder() method or changing the visibility. This method is widely used in the Model and probably in the many users' implementations. It would be a "cosmetic" change that would make users' life harder without any (really good) reason. At least this is how I see it.

[paulbalandan]

I am not a fan of having an extra property of BaseBuilder for the raw table name which is quite different from the processed FROM table. It might cause confusion which is the actual table name used in queries by BaseBuilder.

I have an alternate idea though not yet tested. In the builder() method, we create a static associative array of BaseBuilder instances where the table names are the keys. The method will lookup the static array and check if the builder is already set for the table name and an instance of BaseBuilder, then return it immediately. If not, create a new instance, store it for future uses in that array, then return it.

[MGatner]

I like the static idea but I'm afraid it could cause too much confusion, as Builders carry "state" with them. Contrived example, but if I use $userModel->builder()->where('id >', 10); that where statement stays with the instance. If the shared instance is static, then when I say $groupModel->builder('users')->where('group_id', 5)->findAll() I'm accidentally applying id > 10.

[MGatner]

@paulbalandan Ball is in your court!

[paulbalandan]

I guess you're right. I haven't considered that far. If that's the case then I suppose this fix is the most appropriate.

[paulbalandan]

Can we at least then mention in the user guide that builder() accepts an optional argument to define a table other than the value of $this->table? So this will not come as a secret or undocumented feature. 🤔