Skip to content
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

feat!: add append and prepend handler instead of reverse execution #630

Merged
merged 2 commits into from Jun 23, 2019

Conversation

tflori
Copy link
Contributor

@tflori tflori commented Jun 22, 2019

see #580

BREAKING CHANGE: since pushHandler() was actually prepending handlers
it is now an alias for prependHandler but popHandler() is still
removing the last handler.

This should make the interface clearer: pushing (append) is now executed at the end while prepend is alias by the deprecated pushHandler() method.

People using the deprecated pushHandler() method and popHandler() should update the implementation to use shiftHandler() and prependHandler().

BREAKING CHANGE: since "pushHandler()" was actually prepending handlers
it is now an alias for prependHandler but "popHandler()" is still
removing the last handler.
@staabm
Copy link
Contributor

staabm commented Jun 22, 2019

IMO we should be very careful, because pushHandler() is one of the most important whoops APIs

@tflori
Copy link
Contributor Author

tflori commented Jun 22, 2019

@staabm I guess that is the reason @denis-sokolov suggested to create new methods and mark this as deprecated.

Because of the reverse execution push was not actually appending handlers as the name indicates. The API is not changing but the naming to clear this out.

Copy link
Collaborator

@denis-sokolov denis-sokolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful PR, thank you!

Could you rename the handlerStack variable to handlerQueue? Since it’s not first in, first out anymore?

@tflori
Copy link
Contributor Author

tflori commented Jun 22, 2019

sure thing

@denis-sokolov denis-sokolov merged commit 3eec353 into filp:master Jun 23, 2019
@denis-sokolov
Copy link
Collaborator

🚀

@filips123
Copy link

filips123 commented Jun 23, 2019

pushHandler() should probably also trigger E_USER_DEPRECATED to inform users who still use it.

Also, shouldn't RunInterface also be updated?

@GrahamCampbell
Copy link
Contributor

Adding methods to an interface is a major breaking change.

@filips123
Copy link

Ok, so it should be added in Whoops 3.0.
What about triggering E_USER_DEPRECATED on pushHandler().

@denis-sokolov
Copy link
Collaborator

The method will always work, there is no need to force users off it.

@filips123
Copy link

But it is deprecated, so it could maybe be removed in some future major version.

@icanhazstring
Copy link

Nice change 👍
I am fine with the depreciation warning. As in dev environment those warnings should pop up but not in production of course.

As an example: Zend did the same with the getServiceLocator method on controllers from v2 to v3. So this practice isn't new. So when it's deprecated, notify users about it using the warning and the @deprecated warning.

@denis-sokolov
Copy link
Collaborator

Beware that these changes have been partially reverted in 2.6.0, we should not have released them in a patch version. Please note the PR and the accomodating work was great, the fault is thoroughly on me. #639 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants