Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Added composite primary key support to JTable. #1606

Merged
merged 4 commits into from Oct 23, 2012

Conversation

ianmacl
Copy link
Contributor

@ianmacl ianmacl commented Oct 16, 2012

No description provided.

@ianmacl ianmacl closed this Oct 17, 2012
@ianmacl ianmacl reopened this Oct 17, 2012
@LouisLandry
Copy link
Contributor

Would you mind double checking the code style on this? The pull tester shows 2 more issues than most other ones, and I'd like to be sure. Marked it for 12.3 regardless. I'd also like to leave it open for a few days in case others want to comment.

@ianmacl
Copy link
Contributor Author

ianmacl commented Oct 17, 2012

I used underscores on protected variables to avoid colliding with columns in the table.

* Object constructor to set table and key fields. In most cases this will
* be overridden by child classes to explicitly set the table and key fields
* for a particular database table.
*
* @param string $table Name of the table to model.
* @param string $key Name of the primary key field in the table.
* @param array $key Name of the primary key field in the table.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should key be mixed?

@ianmacl
Copy link
Contributor Author

ianmacl commented Oct 18, 2012

You are right @eddieajau. I have changed it to mixed.

* @param JDatabaseDriver $db JDatabaseDriver object.
*
* @since 11.1
*/
public function __construct($table, $key, JDatabaseDriver $db)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason you are dropping the type hinting here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To save disk space. :P

@mahagr
Copy link
Contributor

mahagr commented Oct 19, 2012

Nice -- but as we are on it, I would like to allow one more use case, which is using "foreign key" as a primary key. For example UCM could be implemented by extending a table with another table using the same key.

I would also make a small feature to allow table to know if item already exists in the table. We have implementation of this feature in Kunena and it would be really easy to add into here, too. Basically you need to add:

protected $_exists = false;

public function exists($exists = null)
{
    $return = $this->_exists;
    if ($exists !== null)
    {
        $this->_exists = $exists;
    }
    return $return;
}

.. and some logic to set the variable when the item gets loaded, saved or removed.

https://github.com/Kunena/Kunena-2.0/blob/master/administrator/components/com_kunena/libraries/tables/kunena.php

Above code also prevents extra lookup to the database if the item was fetched earlier. If you want to handle both, I would set $_exists = null at first and use true/false when you want to know the status of the object (also change the code to make SQL query if state is unknown).

@ianmacl
Copy link
Contributor Author

ianmacl commented Oct 22, 2012

@mahagr Those are good ideas and definitely worth exploring. I'd like to get things in as is first and then we can build on it. I think what is there provides a good foundation that we can build on.

Correct a class name JLoggerCallback => JLogLoggerCallback
@ianmacl ianmacl closed this Oct 23, 2012
@ianmacl ianmacl reopened this Oct 23, 2012
@ianmacl
Copy link
Contributor Author

ianmacl commented Oct 23, 2012

Rebased, squashed and fixed doc block issues.

LouisLandry added a commit that referenced this pull request Oct 23, 2012
Added composite primary key support to JTable.
@LouisLandry LouisLandry merged commit 586e826 into joomla:staging Oct 23, 2012
@ianmacl
Copy link
Contributor Author

ianmacl commented Oct 25, 2012

@mahagr I hope you follow up with a pull request with some of the ideas that you had.

@mahagr
Copy link
Contributor

mahagr commented Oct 26, 2012

@ianmacl Our house is currently under repair (water pipes...), which unfortunately has a small distracting effect on everything I (try to) do.

Are you coming to world conference?

@rashidul0405
Copy link

doesn't included with Joomla 2.5.7 👎

@drmmr763
Copy link

drmmr763 commented Nov 7, 2012

@rashidul04 this is the platform, so the CMS may or may not implement the platform changes back to the CMS. If they do, it usually takes longer, and will not make it into the 2.5.x series.

@pasamio
Copy link
Contributor

pasamio commented Nov 8, 2012

It'll be in Platform 12.3's release which will likely be picked up with the Joomla CMS 3.1 release. You can ship it and load it yourself with 2.5/3.0 and just give it another name so that it doesn't conflict.

@mbabker
Copy link
Contributor

mbabker commented Feb 2, 2013

Unfortunately, it looks like there was an unintended regression here dealing with the asset handling in JTable::store(). It looks like the problem can be chased down to https://github.com/joomla/joomla-platform/blob/staging/libraries/joomla/table/table.php#L719 where the $this->_tbl_key was changed to $this->_tbl_keys since JDatabaseDriver::insertObject() expects that param to be a string, not an array. This causes the ID (or whatever the key(s) may be) of the newly inserted record to not be stored in the JTable instance, meaning when $this->_getAssetName(); is called a few lines later, the last part of the name is .0 instead of the value of the key.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants