whoops: BC Break in 2.4.0
I believe version 2.4.0 contains BC Break (because of https://github.com/filp/whoops/pull/630) in method:
Because internal storage has been changed from stack to queue (reversed order) this method return now reversed order of handler than in version prior to 2.4.0.
Code to reproduce:
$run->pushHandler($handler1);
$run->pushHandler($handler2);
assertSame([$handler1, $handler2], $run->getHanlders());
Works on <2.4.0 and fails on 2.4.0+.
/cc @tflori
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Comments: 23 (8 by maintainers)
Commits related to this issue
- Merge branch 'fix-stack-queue' — committed to filp/whoops by denis-sokolov 5 years ago
A BC revert is a fairly standard approach, applicable to a patch release.
From what I see, 2.4 has been out since August, so the risk of reliance on the new mechanism is largely outweighed by existing clients (which may not even have attempted an upgrade yet).
Yes, it’s a nasty situation, but re-scheduling for a new major (or calling a new major now) are both viable.
I now agree that the original call to include the change in 2.4.0 was a mistake, and I apologize for it and the inconvenience it has caused. In 2.6.0 we have now reverted to the pre-2.4.0 behavior of pushHandler.
Users relying on the new functionality from 2.4.0-2.5.1 will suffer a breaking change. We still have appendHandler and prependHandler, but shiftHandler is missing. Hopefully this will be a good indicator to bring the attention to the unfortunate changes. @tflori
Thank you, everybody, for actively participating and sharing your perspectives, this has helped me a lot.
@denis-sokolov I think there is really a use case that is now broken to admit. For example you start a function that you want to log different when an error occurs. Using the terms as they where before the change: you push a new handler in the stack, execute your subroutine and pop out the last pushed handler. With this change you will pop out the ffirst handler that got pushed and therefore the default handler and not the specified handler.
It may as well be a bug fix, but should probably be reverted for the stable release (due to the extent of the BC break) and only land in a new major release at this point
One more argument that it is BC Break and not a bugfix is that in PR https://github.com/filp/whoops/pull/630 tests were changed - adjusted to the new feature - so sometimes instead of
pushHandlermethodprependHandler orappendHandler` was used.It is showing only that the previous behaviour was desired, documented and well tested. Change from PR #630 as noted by the PR author was BC break, and can’t understand it was completely ignored.