Skip to content

ManyManyList not sortable by many_many_extrafield #2927

Closed
g4b0 opened this Issue Mar 6, 2014 · 8 comments

5 participants

@g4b0
g4b0 commented Mar 6, 2014

To reproduce try installing https://github.com/bummzack/sortablefile , try the setup for many_many, https://github.com/bummzack/sortablefile#example-setup-for-many_many and you will notice that the following function does not return a sorted List

public function SortedImages(){
    return $this->Images()->Sort('SortOrder');
}

I've raised a PR for the above module, so the sorting algorithm is now working (if the PR will be merged), but to have a sorted list into the template the function above should be modified as following:

public function SortedImages() {
    $manyMany = $this->getManyManyComponents('Images');
    $retVal = new ArrayList($manyMany->toArray());
    $retVal = $retVal->sort('SortOrder');
    return $retVal;
}
@simonwelsh simonwelsh added the 3.1 label Mar 15, 2014
@g4b0
g4b0 commented Mar 17, 2014

Here you are a PR with a UnitTest for checking the problem: #2964

@g4b0
g4b0 commented Mar 18, 2014

Following the UnitTest for demonstrating the problem:

public function testSortingByExtrafields() {

        $team1 = $this->objFromFixture('DataObjectTest_Team', 'team1');

        $player1 = new DataObjectTest_Player();
        $player1->Name = 'First';
        $player1->write();
        $team1->Players()->add($player1, array('Position' => 'Defender'));

        $player2 = new DataObjectTest_Player();
        $player1->Name = 'Second';
        $player2->write();
        $team1->Players()->add($player2, array('Position' => 'Goalkeeper'));

        $player3 = new DataObjectTest_Player();
        $player1->Name = 'Third';
        $player3->write();
        $team1->Players()->add($player3, array('Position' => 'Attacker'));

        $first = $team1->Players()->First();        
        $pos1 = $first->Position;
        $id1 = $first->ID;
        $name1 = $first->Name;

        $sortedFirst = $team1->Players()->sort('Position', 'ASC')->First(); 
        $pos2 = $sortedFirst->Position;
        $id2 = $sortedFirst->ID;
        $name2 = $sortedFirst->Name;

        $this->assertEquals('Attacker', $pos2);
    }
@Zauberfisch

I use extra fields to sort many_many quiet a lot, and have never experienced any problems or undefined behavior.

the presented test case is unfortunately invalid and the error does not occur because of a framework issue, instead the test data does not return what I think you assumed.

when I run this test in my setup, the $team1->Players() actually contains 5 items. but you only add 3 in your test case. that means 2 items already exist in the list.
when I debug the test with the following code:

foreach ($team1->Players()->sort('Position', 'ASC') as $p) {
    echo "DataObjectTest_Player #{$p->ID} is at Position '{$p->Position}'<br>";
}

the output is:

DataObjectTest_Player #1 is at Position ''
DataObjectTest_Player #4 is at Position ''
DataObjectTest_Player #8 is at Position 'Attacker'
DataObjectTest_Player #6 is at Position 'Defender'
DataObjectTest_Player #7 is at Position 'Goalkeeper'

this explains why the test fails with the message Failed asserting that null matches expected 'Attacker'., because $team1->Players()->sort('Position', 'ASC')->First() has a Position of null.

Also I noticed some issues in the test case (unused code, wrong assignments), so I rewrote the test case:

public function testSortingByExtraFields() {
    $team1 = $this->objFromFixture('DataObjectTest_Team', 'team1');
    foreach ($team1->Players() as $player) {
        // set all existing players Position to Player
        $team1->Players()->add($player, array('Position' => 'Player'));
    }

    // create a new player with Position Trainer (should be last when sorting Position)
    $player = new DataObjectTest_Player();
    $player->write();
    $team1->Players()->add($player, array('Position' => 'Trainer'));

    // create a new player with Position Captain (should be first when sorting Position)
    $player = new DataObjectTest_Player();
    $player->write();
    $team1->Players()->add($player, array('Position' => 'Captain'));

    $sortedPlayers = $team1->Players()->sort('Position', 'ASC');
    $this->assertEquals('Captain', $sortedPlayers->First()->Position);
    $this->assertEquals('Trainer', $sortedPlayers->Last()->Position);

    // just to prove my point, lets also test for false positives
    $lastCreatedPlayer = $team1->Players()->sort('ID', 'ASC')->Last();
    $this->assertEquals('Captain', $lastCreatedPlayer->Position);
}

this test case now passes on my local environment, so I am guessing this is not actually a bug. could you please run this test on your local environment?

@Zauberfisch

also, I tested ArrayData->sort() vs DataList->sort(), they return exactly the same results, even if records with a Position of null are in the list. (Tested with master)
So i am not sure the pull request for the sortable file module makes sense.

What version of SilverStripe did you use?
did you have other modules or custom code in place that could cause the unexpected behavior?

@g4b0
g4b0 commented Apr 7, 2014

Mmm, your test is passing also in my local envirnment, but I don't understand why my test is still not passing, since the code is pretty the same...

@Zauberfisch

I beg to differ, there are several notable differences between the 2 testcase and yours contains some logical errors which I have outlined in the message.

@kinglozzer
SilverStripe Ltd. member

@g4b0 can this be closed?

@dhensby dhensby closed this Mar 26, 2016
@dhensby
SilverStripe Ltd. member
dhensby commented Mar 26, 2016

Yes ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.