thephpleague / factory-muffin

Enables the rapid creation of objects for testing
https://factory-muffin.thephpleague.com/
MIT License
531 stars 72 forks source link

Potential Laravel forceDelete issues #301

Closed alexhouse closed 10 years ago

alexhouse commented 10 years ago

Been talking to @scottrobertson about this, not entirely sure why this is happening, but when setting the delete method to forceDelete using:

Factory::setDeleteMethod('forceDelete');

I'm getting some odd errors. Although Factory::deleteSaved() does actually delete the saved models, it still ends up throwing exceptions:

We could not delete the model of type: 'User'.

Stack trace:

#0 /home/vagrant/Code/vendor/league/factory-muffin/src/Facade.php(87): League\FactoryMuffin\Factory->deleteSaved()
#1 /home/vagrant/Code/app/tests/TestCase.php(11): League\FactoryMuffin\Facade::__callStatic('deleteSaved', Array)
#2 /home/vagrant/Code/app/tests/TestCase.php(11): League\FactoryMuffin\Facade::deleteSaved()
#3 [internal function]: TestCase::tearDownAfterClass()
#4 /home/vagrant/Code/vendor/phpunit/phpunit/src/Framework/TestSuite.php(708): call_user_func(Array)
#5 /home/vagrant/Code/vendor/phpunit/phpunit/src/Framework/TestSuite.php(703): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult))
#6 /home/vagrant/Code/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(423): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult))
#7 /home/vagrant/Code/vendor/phpunit/phpunit/src/TextUI/Command.php(186): PHPUnit_TextUI_TestRunner->doRun(Object(PHPUnit_Framework_TestSuite), Array)
#8 /home/vagrant/Code/vendor/phpunit/phpunit/src/TextUI/Command.php(138): PHPUnit_TextUI_Command->run(Array, true)
#9 /home/vagrant/Code/vendor/phpunit/phpunit/phpunit(54): PHPUnit_TextUI_Command::main()
#10 {main}

I'm wondering if it has something to do with how you check for return value from firing the delete method here: https://github.com/thephpleague/factory-muffin/blob/d81fde9e8437c9ed74fa6d7137a2cb639767e515/src/Factory.php#L461 forceDelete doesn't actually return a value, so perhaps this is why the exception gets thrown, despite the fact the model actually gets deleted?

alexhouse commented 10 years ago

In fact, changing that line to: if (false === $this->delete($object)) { fixes the issue I'm seeing.

scottrobertson commented 10 years ago

@GrahamCampbell this seems to be the same issue with ->save() with laravel being retarded and not returning booleans when it has already been deleted?

alexhouse commented 10 years ago

https://github.com/laravel/framework/blob/b45f46c7877b0799d0f127eac611ad6c5afd5705/src/Illuminate/Database/Eloquent/SoftDeletingTrait.php#L27

GrahamCampbell commented 10 years ago

This issue has already been solved. Please see the previous issues on this repo.

GrahamCampbell commented 10 years ago

Oh crap. I never implemented the custom stuff for the delete method. I forgot. Give me a min...

scottrobertson commented 10 years ago

:P

alexhouse commented 10 years ago

Great success!

GrahamCampbell commented 10 years ago

@scottrobertson We don't expect the save method to return a boolean in laravel btw. We expect an int for the number of rows modified. That's why I use an == comparison.

scottrobertson commented 10 years ago

Ah right

GrahamCampbell commented 10 years ago

@alexhouse Here's the new usage then:

First of all, you no longer will be calling:

Facade::setDeleteMethod('forceDelete');

Now you can call:

Facade::setCusomDeleter(function ($object) {
    $object->forceDelete();
    return true;
})

This will now call force delete on all your objects, and will return true to satisfy our quick checks.

//cc @scottrobertson

GrahamCampbell commented 10 years ago

You can install the new version with:

{
    "require-dev": {
        "league/factory-muffin": "2.1.*@dev"
    }
}
alexhouse commented 10 years ago

Perfect, thanks @GrahamCampbell !