This
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Table/Table.php#L132
is not compatible with:
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Table/Table.php#L312
and I see the same in 4.0-dev
Table::getInstance($type, $prefix);
should return table instance.
Too few arguments to function Joomla\CMS\Table\Table::__construct()
cause getInstance()
is creating new instance providing only $db
as a parameter. When Table::__construct()
requires to provide $table, $key, $db
.
At least 3.9 to 4.0-dev
Should we update Table::getInstance()? From:
return new $tableClass($db);
to:
return new $tableClass($this->_tbl, $this->_tbl_key, $db);
Just to be clear. You do know that it is counter intuitive and at least doubtful it terms of industry standards?
Just to be clear. You do know that it is counter intuitive and at least doubtful it terms of industry standards?
To make it work, you would have to change every call to Table::getInstance()
to provide the table name and PK, either as separate parameters or stuffing it in an undocumented options array as much of the Joomla API is designed around (because who needs dependency injection or good documentation).
This might be counter intuitive from a purely code review perspective, but making a code change that changes a behavior that has existed in the API for 10 years and is a massive B/C break with no simple mitigation is actually pretty crappy. You end up with one of two solutions:
class MyTableRealCode extends JTable {}
if (version_compare(JVERSION, '4.0', '>=') {
/**
* Joomla 4 version of my table class
*/
class MyTable extends MyTableRealCode
{
public function __construct(string $table, $tableKey, JDatabaseDriver $db)
{
parent::__construct($table, $tableKey, $db);
// Do stuff
}
}
} else {
/**
* Joomla 3, 2.5, and 1.5 version of my table class
*/
class MyTable extends MyTableRealCode
{
public function __construct(JDatabaseDriver $db)
{
parent::__construct('table_name', 'id', $db);
// Do stuff
}
}
}
Or...
class MyTable extends JTable
{
public function __construct()
{
$args = func_get_args();
// If given 3 args, assume Joomla 1.5 thru 3 compatibility, otherwise assume Joomla 4 compatibility
if (count($args) === 1) {
parent::__construct('table_name', 'id', $args[0]);
} elseif (count($args === 3) {
// PHP 5.6 is your minimum supported version, right?
parent::__construct(...$args);
} else {
// No idea how to handle this, bail out or the class is in an invalid state, assuming you actually do error handling in your code
throw new RuntimeException('Yeah, see why this is a bad idea?');
}
}
}
That would be way to far. Why not use build legacy class? I mean keep the current behavior in JTable
and fix the new Joomla\CMS\Table\Table
so it works as it should? Use namespaced class in new components or components released in 4.0 with a correct usage, while giving developers time to migrate. Then drop support for JTable in next major update (5.0?)
You can't do anything with the constructors of the table classes without causing problems for anyone creating instances of those classes, in essence you leak internal implementation details if you must change every call of new Joomla\CMS\Table\Extension($db)
to new Joomla\CMS\Table\Extension('#__extensions', 'extension_id', $db)
. The other option is remove the table name and PK from the base class' constructor and require all subclasses set those properties in their constructors (also a major B/C break).
Long and short, changing this part of the table API has little practical benefit. TBH, time is better spent supporting @wilsonge in the development of https://github.com/joomla-framework/entities so that the Table API can be deprecated.
Status | New | ⇒ | Discussion |
What Michael said in short.
Doing this change would break basically every extension out there I'm afraid, so it's basically not going to happen any time soon even with J4. Working on a full blown replacement using full entities has started and will continue that will indirectly solve this
Status | Discussion | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-03-21 15:05:52 |
Closed_By | ⇒ | wilsonge |
Changing this would be a major B/C break because the status quo is every Table subclass has a constructor with only the database driver as an argument and the table name and PK are provided in a
parent::__construct()
call from the subclass. As the Table class itself is abstract, there is no real issue here as it is impossible to actually trigger an error based on the existing convention.