BC Break in 2.4.0

[michalbundyra]

I believe version 2.4.0 contains BC Break (because of https://github.com/filp/whoops/pull/630) in method:

https://github.com/filp/whoops/blob/cde50e6720a39fdacb240159d3eea6865d51fd96/src/Whoops/Run.php#L130-L133

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

[GrahamCampbell]

It was argued that this was a bug fix (and by definition all bug fixes are BC breaks).

[michalbundyra]

@GrahamCampbell I would then expect the same bugfix in v1, unless it is no longer supported ❓

I see that not only getHandlers behaviour changed, but also for example popHandler:

• before it returns the last attached handler
• now it returns the first

(still I am saying about case of using pushHandler).

[GrahamCampbell]

I think v1 is no longer getting bug fixes.

[denis-sokolov]

Indeed, as Graham has said, we considered this to be a bug fix, and version 1 does not receive updates.

Sorry for the inconvenience this change cuased. Is there a specific use case where this change breaks your code in a way that can’t be reconciled with the new behavior? Feel free to describe your use case and reopen the issue, then!

[michalbundyra]

@denis-sokolov @GrahamCampbell

I am getting errors in zend-expressive out of the box because of changed behaviour of the whoops library. I've checked it closely and considering something like that as a bug fix is a mistake, imho.

All examples where clearly showing that library is using stack (yeah, internal implementation doesn't matter) - see examples in example directory. Also description is saying: "Flexible, stack-based error handling".

Looking at the changes, original code, and the library history this behaviour was there by design. If you have changed your mind and you think it would be better to work the other way (use FIFO instead LIFO) it should be next major version.

Releasing "the bugfix" was clearly a feature (release 2.4.0 not 2.3.x and changelog notes: "Allow to prepend and append handlers").

What's more - if it is a bugfix it should not deprecate any method from public interface - but it did - "pushHandler" is deprecated now.

Now we have situation that when calling "pushHandler" and then "popHandler" give different result (assume that it was not the first handler).

[michalbundyra]

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 pushHandler method prependHandler 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.

[Ocramius]

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

[denis-sokolov]

This is a tough one. If anyone has use cases that have been broken by this, please clarify them, I’d love to see them to make the best decision for everyone.

[michalbundyra]

@denis-sokolov

One:

$handler1 = ...;
$handler2 = ...;

$run->pushHandler($handler1);
$run->pushHandler($handler2);

self::assertSame([$handler1, $handler2], $this->getHandlers());

Two:

$handler1 = ...;
$handler2 = ...;

$run->pushHandler($handler1);
$run->pushHandler($handler2);

self::assertSame($handler2, $run->popHandler());

and probably more.

There were working before correctly, Changes internal implementation should not change the public interface/behaviour which was stable for very long time, and was desired and documented behaviour.

Adjusting tests just to work with new feature is unacceptable if we want to follow semver.

I believe it is still possible to provide this feature in v2 and keep the BC at the same time. I can provide PR with proper changes.

[denis-sokolov]

@webimpress, thank you for the kind explanation of what the backwards compatibility is.

Your code example is not a use case, it’s a code example. I’m looking for real examples of how the users actually used the application and how it broke for them.

[michalbundyra]

@denis-sokolov

Your code example is not a use case

How come? Is it not part of the public interface, documented and tested? I can make any use of the public interface, as it is public...

See my related PR - #640. This is how the feature should be added, imho.

[tflori]

Sorry for the confusion.. it's just important that you can add a handler to be executed after all handlers that are currently registered and remove them from both sides. The naming then is maybe confusing.

[michalbundyra]

@tflori So then I would suggest adding the following methods:

• removeFirstHandler (to remove handler which will be executed as first), and
• removeLastHandler (to remove handler which will be executed as last).

The consumer of the library shouldn't really care what is the internal implementation, if it is stack, queue or whatever.

[tflori]

@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.

[tflori]

@webimpress again: sorry for the confusion. in such a case I would recomment to just change popHandler to shiftHandler. do you have now a use case where you need to get the handlers in (using getHandlers) in the exact same order as before?

[michalbundyra]

@tflori I know that update path is easy, but it shouldn't happen in minor release. Your example clearly show that is BC Break, and a valid use case, imo. Please note that shiftHandler is new function so I need different code to use <2.4.0 and different to use 2.4.0+.

Please see PR I've created and let me know what do you think - #640.

[tflori]

@webimpress the problem is: what is released is released - rolling back the change now will break at least my use case (what was the reason for #630)

You could fix your problem with a simple if:

if (method_exists($run, 'shiftHandler')) {
  $run->shiftHandler();
} else {
  $run->popHandler();
}

Yes, I'm aware that his is very bad and ugly. I also expected version 3 with my breaking change to stick to semantic versioning. Your MR reverts the BC indeed but it creates a new BC to >2.4.

[Ocramius]

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.

[denis-sokolov]

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.

[tflori]

I'm fine with reverting the change and It's absolutely correct to have it not in version 2. What I don't agree is that it is a good idea to remove shiftHandler. I have again no proper solution for my scenario:

// a handler that writes into log file and should always be the first handler
$defaultHandler = new LogHandler();
$whoops = new Run();
$whoops->appendHandler($defaultHandler);

// later in the code...
// I don't know what this handler is doing
// I can't execute it first because it may return Handler::QUIT or Handler::LAST_HANDLER
$anotherHandler = $whatEverObject->getLogHandler();
$whoops->appendHandler($anotherHandler);
// execute the code that requires this handler
$whatEverObject->exec();
// remove the last handler
$whoops->shiftHandler();

// continue the execution loop with different error handlers or what ever...

I hope it get's clear that this is a use case that now again requires an ugly workaround:

$whoops->clearHandlers();
$whoops->appendHandler($defaultHandler);

The thing is that default handlers could be more than one and the custom handlers could also be more than one. This was just for simplification. Currently I don't have more than one handler for each.

[denis-sokolov]

I’m happy to add shiftHandler back, but I would like to add it under a different, very explicit name. Would removeLastHandler (and removeFirstHandler) make sense for you?

[tflori]

sure, it sounds more reasonable

[denis-sokolov]

See 2.7.0 with removeFirstHandler and removeLastHandler.