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
Ticket/11995 Fix Revert of config.remove migration #1847
Conversation
PHPBB3-11995
{ | ||
$this->tool->reverse('remove', 'foo'); | ||
} | ||
catch (Exception $e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an explicit catch necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its just a copy from all the code around, should I change all of them?
try | ||
{ | ||
$this->tool->add('foo', 'bar'); | ||
$this->fail('Exception not thrown'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this test removed? We're trying to make sure an exception is thrown when adding a config var that already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot, exceptions are not thrown for this reason anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you expect an exception, you should be using the @expectedException
annotation if possible. If an exception is unexpected, phpunit should turn them into a failure automatically.
Ticket/11995 Fix Revert of config.remove migration
http://tracker.phpbb.com/browse/PHPBB3-11995