Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relation conditions aren't being applied when saving models #4952

Closed
Flynsarmy opened this issue Feb 20, 2020 · 10 comments · Fixed by octobercms/library#536
Closed

Relation conditions aren't being applied when saving models #4952

Flynsarmy opened this issue Feb 20, 2020 · 10 comments · Fixed by octobercms/library#536

Comments

@Flynsarmy
Copy link
Contributor

Flynsarmy commented Feb 20, 2020

  • OctoberCMS Build: 464
  • PHP Version: 7.4.2

Description:

If you have two belongsToMany relations on the same table with a conditions
value set, record retrieval ($record->myRelation) works correctly and applies
the specified conditions, however saving $record wiil result in the first
relation being wiped and only the second relation will save.

newPivotQuery() isn't applying constraints as it should.

Steps To Reproduce:

  • git clone https://github.com/Flynsarmy/oc-relationtest-plugin.git to /plugins/flynsarmy/relationtest
  • php artisan plugin:refresh Flynsarmy.RelationTest
  • Go to /backend/flynsarmy/relationtest/posts/update/1
  • Notice we have two categories in our category relation and two tags in our tags relation. Both relations connect to the same table but use a conditions option in our model.
  • Hit Save
  • Refresh
  • Categories has been wiped but Tags saved.

Issue in Depth

In our Post model we have two relations defined:

    public $belongsToMany = [
        'tags' => [
            'Flynsarmy\RelationTest\Models\Term',
            'table' => 'flynsarmy_relationtest_posts_terms',
            'key'        => 'post_id',
            'otherKey'   => 'term_id',
            'conditions' => 'type = "tag"'
        ],
        'categories' => [
            'Flynsarmy\RelationTest\Models\Term',
            'table' => 'flynsarmy_relationtest_posts_terms',
            'key'        => 'post_id',
            'otherKey'   => 'term_id',
            'conditions' => 'type = "category"'
        ],
    ];

These link to the same table but retrieval and display on our form works as
expected thanks to our conditions statement. This can be further be confirmed
with

php artisan tinker
Flynsarmy\RelationTest\Models\Post::first()->tags
Flynsarmy\RelationTest\Models\Post::first()->categories

When saving the model, October\Rain\Database\Relations\BelongsToMany::setSimpleValue()
calls $this->sync($value);.
The sync method defined in /vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php
has the lines

    $current = $this->newPivotQuery()->pluck(
        $this->relatedPivotKey
    )->all();

to determine which records need to be saved for the current relation. The problem is
newPivotQuery() doesn't apply our conditions, so the query

select * from `flynsarmy_relationtest_posts_terms` where `post_id` = ?

is executed and ALL records in this table are returned instead of just the ones
relevent to the current relation.
The next lines in this class:

$detach = array_diff($current, array_keys(
    $records = $this->formatRecordsList($this->parseIds($ids))
));

detect which records weren't selected on our form and lists them for removal.

Because our form has the Categories field first, it marks the tag records for removal.
Removal occurs, and we move on to the Tags field. The same thing happens and our
categories are marked for removal.

The end result is only the tags are saved and all categories are removed.

@github-actions
Copy link

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@Flynsarmy
Copy link
Contributor Author

Ping

@github-actions
Copy link

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@Flynsarmy
Copy link
Contributor Author

Ping

@github-actions
Copy link

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@Flynsarmy
Copy link
Contributor Author

Ping

@LukeTowers LukeTowers added this to the v1.0.468 milestone Jun 26, 2020
@bennothommo bennothommo modified the milestones: v1.0.468, v1.0.469 Jul 29, 2020
@bennothommo bennothommo self-assigned this Sep 3, 2020
@LukeTowers LukeTowers modified the milestones: v1.1.0, v1.1.1 Sep 4, 2020
@mjauvin
Copy link
Contributor

mjauvin commented Oct 26, 2020

@Flynsarmy I was able to reproduce this using your relationtest plugin.

Two questions:

  1. did you try the same with a scope instead of a condition (see here)?
  2. should you not use polymorphic relations to achive this?

Note: I'm not saying this is not a bug, just exploring alternate options in the mean time.

@mjauvin
Copy link
Contributor

mjauvin commented Oct 26, 2020

Update: I tried using scopes as following (same behavior as with conditions):

    public $belongsToMany = [
        'tags' => [
            'Flynsarmy\RelationTest\Models\Term',
            'table' => 'flynsarmy_relationtest_posts_terms',
            'key'        => 'post_id',
            'otherKey'   => 'term_id',
            'scope' => 'isType'
        ],
        'categories' => [
            'Flynsarmy\RelationTest\Models\Term',
            'table' => 'flynsarmy_relationtest_posts_terms',
            'key'        => 'post_id',
            'otherKey'   => 'term_id',
            'scope' => 'isCategory'
        ],
    ];

And adding the scopes to the Term class:

public function scopeIsCategory($query)
{
   $query->where('type', 'category');
}

public function scopeIsTag($query)
{
   $query->where('type', 'tag');
}

@mjauvin
Copy link
Contributor

mjauvin commented Oct 26, 2020

@mjauvin
Copy link
Contributor

mjauvin commented Oct 28, 2020

@LukeTowers I confirm this IS a bug, please update the status type.

Thanks.

LukeTowers added a commit to octobercms/library that referenced this issue Nov 20, 2020
Fixes: octobercms/october#4952

Co-authored-by: Luke Towers <github@luketowers.ca>
Co-authored-by: Ben Thomson <git@alfreido.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants