unlink and link should also use the via on conditions of the relation

[Bhoft]

Hi
I have a ManyMany relation between 2 Tables e.g. ReviewerInvites and Submissions.
I have an allocation of "multiple" reviewer Types e.g. individual review, consensus review.

Therefore i have an additional column "reviewer_type" in the junction Table.
and defined constants like :

    REVIEW_TYPE_INDIVIDUAL = 1
    REVIEW_TYPE_CONSENSUS = 2

The Junction Table has the following columns,
submission_id, reviewer_invite_id, reviewer_type

I could now have the relation in my Submission Model like this:

    public function getConsensusReviewer()
    {
        return $this->hasMany(ReviewerAllocation::className(), ['submission_id' => 'id'])
            ->andOnCondition(['reviewer_type' => self::REVIEW_TYPE_CONSENSUS]);
    }

    public function function getConsensusReviewers()
    {
        return $this->hasMany(Reviewer::className(), ['id' => 'reviewer_id'])
             ->via('consensusReviewer');
    }

the same for the individual reviewers

    public function getIndividualInvites()
    {
        return $this->hasMany(ReviewerInvite::className(), ['id' => 'reviewer_invite_id'])
            ->via('individualReviewer');
    }
    public function getIndividualReviewer()
    {
        return $this->hasMany(ReviewerAllocation::className(), ['submission_id' => 'id'])
            ->andOnCondition(['reviewer_type' => self::REVIEW_TYPE_INVIVIDUAL]);
    }

Everything works except "link()" and "unlink()".

For "link()" i could use the "extraColumns" attribute to set the reviewer_type.
But when "unlink()" is used the On condition isn't used.

The SQL always deletes me entries for both reviewer_types:
e.g.

    DELETE FROM `submission_reviewer_invite` WHERE (`submission_id`=70) AND (`reviewer_invite_id`=47) 

instead of:

    DELETE FROM `submission_reviewer_invite` WHERE (`submission_id`=70) AND (`reviewer_invite_id`=47) AND (`reviewer_type`=2)

I also tried to use viaTable instead of via and set the onCondition directly.

    public function getIndividualInvites()
    {
        return $this->hasMany(ReviewerInvite::className(), ['id' => 'reviewer_invite_id'])
	  		->viaTable('submission_reviewer_invite', ['submission_id' => 'id'], function ($query) {
            $query->onCondition(['reviewer_type' => self::REVIEW_TYPE_INVIVIDUAL]);
        });
    }

Same result.
The additional attribute must be set via extraColumns for link() but for unlink() again the onCondition isn't used. And there is no other way then to create an manual find()->where()->delete() request.

Which lets you to the point that several ManyMany releation extensions couldn't been used in this case all which uses unlink() for deleting the relations.
E.g.
la-haute-societe/yii2-save-relations-behavior#16
https://github.com/yii2tech/ar-linkmany

So I wonder why the viaRelation on and where conditions aren't used in e.g. unlink()?
https://github.com/yiisoft/yii2/blob/master/framework/db/BaseActiveRecord.php#L1408

Where something like (after the line above) below would solve this issue:

            foreach ($viaRelation->on as $key => $value) {
                $columns[$key] = $value;
            } 

Edit:
correct position is above these line because otherwise the key would go into the nulls array.
https://github.com/yiisoft/yii2/blob/master/framework/db/BaseActiveRecord.php#L1413

Something similar should also be done for link() of course.

I mean if i link between some models where some additional condition is used this should be a default value of the entry.

And if you unlink no other then the related entries should be deleted.

I have seen that this kind of request was done multiple times:
#14598
#4932

Additional info

Q	A
Yii version	2.0.16?
PHP version	7.1.x
Operating system	Windows / Linux

[alexkart]

As discussed here #14598 it seems that it shouldn't be fixed as extraColumns parameter is not meant to be used like this.
@samdark, shouldn't this and related issues be closed?

[Bhoft]

I don't want to use any extraColumns at all.
If the relation has onCondition parameters those should automatically used for link() and unlink().
That would make the extaColumns really only be used for additional columns which is not some secondary key value.

[papppeter]

Hi @samdark , @Bhoft

do we have any progress here? we just faced the same problem #17197 pull request try to solve. It would be great to have a nice solution.

we are using heavily relation saver module, which makes life much easier. but because of this it is not working fully.

[samdark]

@pappfer no, no progress on this one. There was a PR #17197 but it doesn't have tests and, it seems, in only a partial solution.

[pappfer]

@samdark I think you meant to tag @papppeter :)

[papppeter]

@samdark do you think somebody from the core team could pick this up?

we checked the problem and the suggested solution as well. and it is not really easy to do it, without a deep knowledge of AR. By the way the solution looks quite close to be a good one.

i think this should be fixed on the long run, it is not doing the expected behaviour. or make i just deprecated so people are aware of this problem prompt.

what do you think?

[samdark]

Only if @bizley has enough time for it.

[papppeter]

Hi @bizley

would you have time to look into this problem? it would be a great help to us!

[bizley]

I'll look into this.

[bizley]

@papppeter could you check if proposed change in BaseActiveRecord fixes your problem?

[papppeter]

@bizley i can do it tomorrow, latest on Monday.

[papppeter]

@bizley this is perfect for us! i checked and it works as we expect.

@samdark i hope this can be released soon.

[samdark]

@pappfer will take some time. Meanwhile you can try using dev-master@dev.

[Bhoft]

@bizley is there a reason why the condition wasn't also added to "link()" as well as i suggested in my PR almost 2 years ago?

i mean if an relation is now correctly unlinked with "all conditions" why should this not be the same for "link" as well. Without the change i always have to set the conditions if i wanted to link between 2 models which are on a specific relation.

[papppeter]

@Bhoft i think that part is just less straight forward, collecting all the values if the on condition has some multi level condition.

Link method has the extraColumns which you can use.

we are using https://github.com/la-haute-societe/yii2-save-relations-behavior which will be now a perfect solution for this problem.

by the way you can always write your link method which prefills the extraColumns.

unlink you couldn’t get it to work the way it should

[Bhoft]

Yes i knew, but to have the correct "relation" between my models i "must" also "prefill" the extra Columns.
I knew this behaviour as well as i also used it 2 years ago.

I also knew that i extend or overload the link function, but in my opinion this shouldn't be a "manual" thing to do.

[bizley]

@Bhoft link() cannot use the same straight-forward implementation for on conditions. What if you use something like onCondition(['>', 'column', 1])? How to fill the column there then? With unlink() we can use it because this is just another "where" condition.

For additional columns info extraColumns are fine and enough in my opinion.

[Bhoft]

Ok but i don't think you would have an relation with an on condition with column > 1 or something similar for crud operations of the models, only for special listings or similar.

I am fine with it, but maybe there is really some special case were it could be useful.