FIX Correctly remove relations with ManyManyThroughList::removeall

[GuySartorelli]

Instead of just setting one side of the relation to null in the through table, remove the rows entirely.
Remove only the relations which match the filters that have already been set on the list.
This is consistent with the way ManyManyList works - in fact much of the code for the tests are taken straight from that class.

The issue was ultimately caused by the HasManyList which is being used as an intermediary not having and of the 'where' clause (i.e. filter/exclude) of the underlying dataquery of the ManyManyThroughList. Changing that without causing regressions seemed like a bit of a tricky way to handle this - and even then if we still relied on using removeAll() from that HasManyList we would have still seen the rows in the through table being updated with null IDs on one side of the relationship instead of those rows being deleted outright.

Also some small tidy-up (removing an unnecessary line break and an unused "use" statement)

Parent Issue

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

[kinglozzer]

Given that the join records are DataObjects in their own right, do you think we’d be better approaching this in a similar way to ManyManyThroughList::removeByID()? It’s a little slower, but deletions are rarer than writes and it does allow for onBeforeDelete/onAfterDelete etc on the join records to be invoked

[GuySartorelli]

Hmm I'm conflicted between wanting to keep it performant and wanting to let people use those extension points...

Would it be reasonable to make it configurable? We could add a new configuration boolean on the join record class (something like private static bool $use_orm_for_removeall - obviously we can't declare this but we can document it, and the default null is falsey so that goes with how I'd want this to work).

Then in removeall if that config is falsey we use this or a similar approach, and if it's true we use the ORM so that any project that explicitly needs those hooks called on a join record can have that functionality available.

Keeps it snappy for the majority but gives flexibility to the minority that need it.

[kinglozzer]

Maintaining two codepaths based on a config flag isn’t ideal 😕

I think we should be consistent with how we handle the “through” records too. When $manyManyThrough->add($item) is called, you can use onBeforeWrite/onAfterWrite hooks, ownership hooks etc, so I think we should offer the same functionality on deletions. I think that was part of the appeal of building the “through” API.

Performance-wise there shouldn’t really be any noticeable difference until you reach hundreds/thousands of join records. That sort of thing tends to be handled by scheduled tasks rather than synchronous user actions.

[kinglozzer]

Just rethinking this, we’re changing behaviour from un-assigning a join record to deleting a join record. For old many-many that’s fine, but given that the join records are full DataObjects in many-many-through is that definitely desirable? I can’t think of a use case off the top of my head where you’d want to keep the unassigned join records, but I suppose there could be one...

Would we be safer to change this PR to just fix the filter issue and leave the “unassign” behaviour in place?

[GuySartorelli]

I think the "unassign" behaviour is categorically a bug. If I call removeAll() I want the records to be removed, as the name suggests - which is consistent with how remove() and removeByID() work - they remove the record outright, which seems to me that's exactly what we'd expect.
I'm happy to be voted down on that if someone feels strongly about it but I really do think remove should remove without ambiguity - especially if we're concerned about making sure the onBeforeDelete() hooks get called (which they obviously won't if we aren't deleting).

I think I'll wait a bit and see if anyone else opines before I make any changes.

[emteknetnz]

It looks good, though could you bump up the number of fixtures used here from 2 to 3. Reason being is so that we can filter out 2 and assert there is 1 remaining, this is just to ensure it's removing 'all' rather than 'first'

Also, if there's not a test already, could you also add a test for ->removeAll() on an unfiltered list just to ensure that it removes everything

[GuySartorelli]

@emteknetnz do you have any opinions on using SQLDelete vs for example looping through and calling remove() to allow the ORM hooks on the join objects to be called?

[GuySartorelli]

Discussed with @emteknetnz offline - he says it makes sense to use the ORM for this situation, so that's two in favour of the ORM. I'll take that approach.

[GuySartorelli]

could you also add a test for ->removeAll() on an unfiltered list just to ensure that it removes everything

That's part of this same testRemoveAll() method - for clarity I'll split out out so there's one method for testing unfiltered revoveAll() and one for filtered.

[GuySartorelli]

NOTE I have also changed the IDs passed into the remove callbacks - the removeCallbacks() method defined in RelationList implies that these should be the IDs of the related items being removed, not the join records. When I added the child0 fixture and ran the unit tests, I got the following errors which clearly indicate that the tests were expecting IDs of the related records and not the join records. I've reviewed ManyManyList and that's using the IDs of the relations.

There were 3 failures:

1. SilverStripe\ORM\Tests\ManyManyThroughListTest::testCallbackOnSetById
   Failed asserting that two arrays are equal.
   --- Expected
   +++ Actual
   @@ @@
   Array (

• 0 => 3

• 0 => 2
  )

/var/www/vendor/silverstripe/framework/tests/php/ORM/ManyManyThroughListTest.php:450
/var/www/vendor/sminnee/phpunit/phpunit:52
/var/www/vendor/bin/phpunit:117

2. SilverStripe\ORM\Tests\ManyManyThroughListTest::testRemoveCallbackOnRemove
   Failed asserting that two arrays are equal.
   --- Expected
   +++ Actual
   @@ @@
   Array (

• 0 => 3

• 0 => 2
  )

/var/www/vendor/silverstripe/framework/tests/php/ORM/ManyManyThroughListTest.php:483
/var/www/vendor/sminnee/phpunit/phpunit:52
/var/www/vendor/bin/phpunit:117

3. SilverStripe\ORM\Tests\ManyManyThroughListTest::testRemoveCallbackOnRemoveById
   Failed asserting that two arrays are equal.
   --- Expected
   +++ Actual
   @@ @@
   Array (

• 0 => 3

• 0 => 2
  )

/var/www/vendor/silverstripe/framework/tests/php/ORM/ManyManyThroughListTest.php:499
/var/www/vendor/sminnee/phpunit/phpunit:52
/var/www/vendor/bin/phpunit:117

[sabina-talipova]

I have approved, but there is some Travis test failed. Could you check it please.

1) SilverStripe\Core\Tests\MemoryLimitTest::testIncreaseMemoryLimitTo
Failed to set memory limit to 67108864 bytes (Current memory usage is 182976512 bytes) 

@emteknetnz, could you have a look, please
https://app.travis-ci.com/github/silverstripe/silverstripe-framework/builds/250242473

[GuySartorelli]

FYI the failure is on the nightly php build which is no longer used in our travis config from 4.10 onwards. Besides which that nightly build is php 8.0 which 4.9 doesn't officially support. I think it's safe to ignore that.

[maxime-rainville]

Looks good at first glance.

Can you just confirm what happens in the following scenarios:

• the same relations is defined multiple times. e.g.: Example Class A has a many-many relation to Class B. One object A is related multiple times to the same Object B.
• What happens if the ManyManyThrouh object is versioned? Do we want to add a unit test specifically for that?

Just want to make sure we've covered all our basis.

[GuySartorelli]

Can you just confirm what happens in the following scenarios:
the same relations is defined multiple times. e.g.: Example Class A has a many-many relation to Class B. One object A is related multiple times to the same Object B.

As mentioned in another comment this isn't really supported - but if you do manage to get a duplicate relation, it is appropriately cleared out with the call to removeAll().

What happens if the ManyManyThrouh object is versioned? Do we want to add a unit test specifically for that?

I don't know what if any thought was put into this scenario when ManyManyThroughList was implemented - it seems that versioning the join class doesn't affect anything at all. The `staged' aka draft version is always used, so publishing and unpublishing updates the version and live tables, but doesn't actually affect the functionality of the list. You always only see the draft relations.

removeAll() and removeByID() also ignore the live and version tables - they only operate on the draft. So you can have a published relation object where there's no draft object - but again, that doesn't affect anything. The list just won't see the relation at all.

Given the above, I think versioning is out of scope for this PR. At most, I could add a note in the docs that versioning the join objects isn't currently supported.

[maxime-rainville]

My pesky question have been addressed.