?
avatar artur-stepien
artur-stepien
6 Feb 2019

Steps to reproduce the issue

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

Expected result

Table::getInstance($type, $prefix); should return table instance.

Actual result

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.

System information (as much as possible)

At least 3.9 to 4.0-dev

Additional comments

Should we update Table::getInstance()? From:

return new $tableClass($db);

to:

return new $tableClass($this->_tbl, $this->_tbl_key, $db);
avatar artur-stepien artur-stepien - open - 6 Feb 2019
avatar joomla-cms-bot joomla-cms-bot - labeled - 6 Feb 2019
avatar mbabker
mbabker - comment - 6 Feb 2019

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.

avatar artur-stepien
artur-stepien - comment - 6 Feb 2019

Just to be clear. You do know that it is counter intuitive and at least doubtful it terms of industry standards?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23779.

avatar artur-stepien
artur-stepien - comment - 6 Feb 2019

Just to be clear. You do know that it is counter intuitive and at least doubtful it terms of industry standards?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23779.

avatar mbabker
mbabker - comment - 6 Feb 2019

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?');
        }
    }
}
avatar artur-stepien
artur-stepien - comment - 18 Feb 2019

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?)

avatar mbabker
mbabker - comment - 18 Feb 2019

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.

avatar franz-wohlkoenig franz-wohlkoenig - change - 3 Mar 2019
Status New Discussion
avatar wilsonge
wilsonge - comment - 21 Mar 2019

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

avatar wilsonge wilsonge - change - 21 Mar 2019
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2019-03-21 15:05:52
Closed_By wilsonge
avatar wilsonge wilsonge - close - 21 Mar 2019

Add a Comment

Login with GitHub to post a comment