Bug: Incorrect type BaseBuilder::$tableName

[iRedds]

Direction Property: string https://github.com/codeigniter4/CodeIgniter4/blob/8322f3aa396ce91b7e56185cc7eb7c8a0dcf8b56/system/Database/BaseBuilder.php#L163-L165

But in the constructor, you can assign an array to the property. (See docBlock) https://github.com/codeigniter4/CodeIgniter4/blob/8322f3aa396ce91b7e56185cc7eb7c8a0dcf8b56/system/Database/BaseBuilder.php#L254-L269

The method returns a string. ( See docBlock and the return typehint) https://github.com/codeigniter4/CodeIgniter4/blob/8322f3aa396ce91b7e56185cc7eb7c8a0dcf8b56/system/Database/BaseBuilder.php#L317-L321

Affected module(s) BaseBuilder аnd maybe somewhere else

[kenjis]

There is no test case to use array for the BaseBuilder constructor param $tableName.

Another array tableName is found in BaseConnection::table(). It seems there is no code to use array to the method. https://github.com/codeigniter4/CodeIgniter4/blob/8322f3aa396ce91b7e56185cc7eb7c8a0dcf8b56/system/Database/BaseConnection.php#L957-L962

It is the primary table name, so it should be string. It seems array is just a mistake. https://github.com/codeigniter4/CodeIgniter4/blob/8322f3aa396ce91b7e56185cc7eb7c8a0dcf8b56/system/Database/BaseBuilder.php#L158-L165

[iRedds]

https://github.com/codeigniter4/CodeIgniter4/blob/622d1bf140fffa62edd59ee79a78a1e173352433/tests/system/Database/Builder/AliasTest.php#L32-L39

[kenjis]

I think that in BaseBuilder::__construct() may assign an array to the property $tableName is a bug.

By the way, what is the primary table? For what?

[kenjis]

The primary table is the table of CodeIgniter\Model object?

[iRedds]

I only found one place where this is used. https://github.com/codeigniter4/CodeIgniter4/blob/35bf87cc050f5619b6ee41ce41b34fc5bf350388/system/Model.php#L563

The content of the method is causing me confusion. If the method should return a shared instance of the BaseBuilder class for this model. Why add the ability to specify a table other than the model table?

[iRedds]

I honestly don't understand why bind a query builder to a table permanently at all. This should be optional.

[kenjis]

Yeah, I think Query Builder's responsibility is to build SQL query. It doesn't need the primary table.

[iRedds]

After looking closely at the BaseBuilder class, I came to the conclusion that this is not a QueryBuilder. This is a ModelQueryBuilder. Methods that change the state of the database do not expect a table name as an argument. (insert, update, delete). Instead of implementing the query builder in the model, the query builder was redesigned for the model.

In this case, the CI3 QueryBuilder will be of better quality than the CI4 QueryBuilder.

[MGatner]

Why add the ability to specify a table other than the model table?

This was a compromise to bring functionality in line with expectations set by the User Guide. It was very confusing to be able to call table() and have the behavior differ form instance to instance.

[MGatner]

To the original issue, I don't think $table should be an array. I would have to look deeper what the alias function for a table is, but I think the rest of framework assumes it's a string and an array will cause failures elsewhere.

[kenjis]

This was a compromise to bring functionality in line with expectations set by the User Guide.

What do you mean? What is the compromise? What functionality?

[iRedds]

CI4 is a cargo cult for Laravel. The db->table() method is a parody of DB::table() written on crutches. We can copy functionality from other frameworks, but do it wisely.

[MGatner]

@kenjis because we already allowed access to the builder() and table() method it wasn't feasible to back out of offering the full function of those (e.g. passing a table name as a parameter to get a table-specific builder). Once the bug was discovered (e.g. $usedModel->builder('widgets') always returned a Builder for users table) we either had to embrace it or create a breaking change to restrict access. I would like version 5 to have much clearer lines between models and the database layer, but that's a ways off.

@iRedds I'm not sure what your comment means, but the reality is that the database layer was almost entirely complete before the current group of maintainers started. We work with what we have, which may not be ideal but it is well documented and consistent. An overhaul to the database layer is definitely on my "wish list" but it is a pretty large task and requires knowledge in a lot of areas.

[kenjis]

@MGatner Thank you for your explanation.

[kenjis]

The db->table() method is a parody of DB::table() written on crutches.

I don't know Laravel. What is the advantage of DB::table() over $db->table()?

[lonnieezell]

CI4 is a cargo cult for Laravel. The db->table() method is a parody of DB::table() written on crutches. We can copy functionality from other frameworks, but do it wisely.

While there has been some changes I missed (like table taking an array) table() had one very simple intent: return a non-shared instance of the query builder. IIRC almost every single other PHP framework has the same method, not just Laravel. It was added because in CI3 you could accidentally intermingle two queries if you had to perform a query to look data up after starting a query elsewhere. By returning a non-shared instance of the QueryBuilder, it stops that from happening. That's all the table method in BaseBuilder does.

Since the Model is a layer of convenience methods, when you call builder() from within the model it assumes that you want a QueryBuilder targeted for the table the model is designed to work with. While within a model, you can still call $this->builder('otherTable') to get a new, non-shared instance of a QueryBuilder for that specific table.

Either way, once you have the new instance, it is the QueryBuilder/Connection that is used through the rest of that call.

In short, though, I don't recall when the ability to have multiple tables specified being added and a very quick glance through the history of BaseConnection didn't make it obvious. Tracing through the code it looks like it likely only supports an array of table names because the BaseBuilder's from method supports multiple tables, which is used in the BaseBullder constructor to set the table after it's called by $db->table('foo').

This makes sense as you can have queries like:

SELECT name, price, options, photo 
FROM food, food_menu 
WHERE food_id = '1'

So it does appear the original post is correct, that the $tableName docblock needs updating, as does the getTable method to support arrays also.

[iRedds]

Difference between CI table() and Laravel table() implementation in:

//CI table()  = 
new BaseBuilder('table', $connection)

// Laravel table()  =
$this->query()->from('table');  //where the query() methed return new QueryBuilder instance;

In Lara, we can get a simple Builder without a predefined table.

Multiple tables can be passed as a string. table('table_1, table_2') from('table_1, table_2') This is already implemented in the builder.

[kenjis]

So it does appear the original post is correct, that the $tableName docblock needs updating, as does the getTable method to support arrays also.

I don't know why the property $tableName should support array.

[kenjis]

@iRedds Thanks for the explanation.

[kenjis]

table() had one very simple intent: return a non-shared instance of the query builder. ... It was added because in CI3 you could accidentally intermingle two queries if you had to perform a query to look data up after starting a query elsewhere. By returning a non-shared instance of the QueryBuilder, it stops that from happening. That's all the table method in BaseBuilder does.

https://github.com/codeigniter4/CodeIgniter4/blob/de8d7f40d799d584d86af2945deac113e1a7c840/system/Database/BaseConnection.php#L962-L971

table() always returns new instance. So the argument $tableName does not matter to return a non-shared instance.

[kenjis]

My current opinion:

The property BaseBuilder::$tableName, BaseBuilder::__construct($tableName), BaseConnection::table($tableName)

• All the $tableName should be ?string.
• It represents the primay table name (single table) if not null.
• It is used in FROM clause in BaseBuilder if not null.

Deprecated

• BaseBuilder::__construct($tableName) as array and string with multiple tables.
• BaseConnection::table($tableName) as array and string with multiple tables.

[iRedds]

What do you think about excluding the $tableName parameter from the constructor and add a public method like defaultTable(), which contains the constructor code that handles $tableName.

[kenjis]

Do you mean remove $tableName from BaseBuilder::__construct()? I think it is good.

But It is BC break. It seems to be difficult to do it in a short term.

[MGatner]

We cannot update method signatures in a way that would break existing extensions.

[kenjis]

@MGatner I understand that we can not break BC in 4.x. But we can do it in 5.0.

[kenjis]

I changed my opinion. I sent a PR #5378.