Inconsistent code while handling CManyManyRelation #636

Closed
klimov-paul opened this Issue Apr 25, 2012 · 3 comments

Comments

Projects
None yet
4 participants
@klimov-paul
Member

klimov-paul commented Apr 25, 2012

"Many-To-Many" relation in active record is specified by the string in following format:

'relationName' => array(CActiveRecord::MANY_MANY, 'RelatedTableName', 'connectorTableName(mainForeignKey, relatedForeignKey)'),

The main critical information about "many-to-many" relationship is specified by the last string parameter, which determines the connector table and foreign keys. This makes "CActiveRecord::MANY_MANY" much different from the other relations.
But just find the declaratoin of the "CManyManyRelation" class (file "system.db.ar.CActiveRecord") - it is empty. The critical information I mentioned is actually stored in "CBaseActiveRelation::foreignKey". That is fine enough, but, when comes to perform the relational queries (for example "CActiveFinder::applyLazyCondition()') this field is parsed using "preg_match", and it is parsed every time, when new query is running.

While I attempt to craete an active record behavior, which handles the saving of "many-to-many" relation data, I found I have to perform the same "preg_match" functionality to retrieve the data I need form "CManyManyRelation" instance. That makes this code to be duplicated.

I suggest the class "CManyManyRelation" should be extended. Methods "CManyManyRelation::getConnectorTableName()" and "CManyManyRelation::getForeighKeys()" should be added. This methods should perform internal "preg_match" call and fill up the internal protected fields with the corresponding data.

class CManyManyRelation extends CHasManyRelation { protected $_connecorTableName = null; protected $_foreighKeys=null;
public function getConnectorTableName() {
    if ($this->_connectorTableName === null) {
         $this->initAdditionalRelationData();
    }
    return $this->_connectorTableName;
}

protected function initAdditionalRelationData() {
      if(!preg_match('/^\s*(.*?)\((.*)\)\s*$/is',$this->foreignKey,$matches)) {
      ...
}

...

}

I am using framework version 1.1.10

@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Apr 27, 2012

Owner

@klimov-paul I've reported the same issue in a different way in #508. your solution looks like the better one on first sight.
btw: I started implementing on a behavior for saving many-many relations which is almost ready for initial release now:
https://github.com/yiiext/activerecord-relation-behavior I am planning to finish 1.0 within the next two weeks.

Owner

cebe commented Apr 27, 2012

@klimov-paul I've reported the same issue in a different way in #508. your solution looks like the better one on first sight.
btw: I started implementing on a behavior for saving many-many relations which is almost ready for initial release now:
https://github.com/yiiext/activerecord-relation-behavior I am planning to finish 1.0 within the next two weeks.

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark May 22, 2012

Owner

@klimov-paul looks like a good thing.

@qiangxue, @mdomba opinions?

Owner

samdark commented May 22, 2012

@klimov-paul looks like a good thing.

@qiangxue, @mdomba opinions?

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba May 22, 2012

Member

It would certainly help to reduce preg_match calls

Member

mdomba commented May 22, 2012

It would certainly help to reduce preg_match calls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment