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

ManyManyThroughList::removeAll() doesn’t respect filters #10104

Closed
1 task done
kinglozzer opened this issue Sep 29, 2021 · 4 comments · Fixed by #10295
Closed
1 task done

ManyManyThroughList::removeAll() doesn’t respect filters #10104

kinglozzer opened this issue Sep 29, 2021 · 4 comments · Fixed by #10295

Comments

@kinglozzer
Copy link
Member

kinglozzer commented Sep 29, 2021

Unlike DataList, HasManyList and ManyManyList, when using many-many-through ManyManyThroughList::removeAll() will discard all filters and remove everything:

// Works as expected, relation is removed for all items except those with IDs in the $processedIDs array
foreach ($venue->AccessOptions()->exclude(['ID' => $processedIDs]) as $option) {
    $venue->AccessOptions()->remove($option);
}

// Removes all related items, including those in the $processedIDs array
$venue->AccessOptions()->exclude(['ID' => $processedIDs])->removeAll();

Acceptance criteria

  • Calling removeAll on a filtered ManyManyThroughList only remove relations for items matching the filter.

PRs

@maxime-rainville
Copy link
Contributor

Probably worth back porting all the way back to 4.9.

@GuySartorelli GuySartorelli self-assigned this Apr 28, 2022
@GuySartorelli
Copy link
Member

only remove items matching the filter

Just to clarify: this doesn't remove the items, it removes the relations - and even then it's not removing the row from the through table, it's just setting the ID for one side of the relation to 0 (which IMO is absolutely not the correct behaviour).

I'm going to update this to use the same logic used by ManyManyList::removeAll() - it will create a new delete SQL query that removes the relevant entries from the through join table.

@sabina-talipova
Copy link
Contributor

@maxime-rainville, I've done tests on my local and ran unit tests as well. I could merge if you don't have any other questions.

@maxime-rainville
Copy link
Contributor

All happy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants