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

[5.5] add assertDispatchedTimes for event fakes #22528

Merged
merged 1 commit into from
Dec 24, 2017
Merged

[5.5] add assertDispatchedTimes for event fakes #22528

merged 1 commit into from
Dec 24, 2017

Conversation

ekateiva
Copy link
Contributor

@ekateiva ekateiva commented Dec 24, 2017

Hey,

I find it useful to test how many times the event was dispatched. I exposed the method assertDispatchedTimes(), however you can pass ant int to assertDispatched() instead of a callback, so you can use Event::assertDispatched(SomeEvent::class, 3).

@taylorotwell taylorotwell merged commit a55c4bf into laravel:5.5 Dec 24, 2017
@sisve
Copy link
Contributor

sisve commented Dec 25, 2017

This is a change in a method signature and a breaking change.

@vlakoff
Copy link
Contributor

vlakoff commented Dec 27, 2017

Actually, there are no signature changes ;)

@GrahamCampbell
Copy link
Member

Actually, there are no signature changes ;)

There is, because the 2nd param's type signature has changed. There are just no static signature changes.

@ekateiva
Copy link
Contributor Author

I am not sure why you count it as a breaking change. Actually, the param has not changed, just the new type of it was introduced. However, the functionality is 100% backwards compatible.

@GrahamCampbell
Copy link
Member

The problem is that someone extending the class, overriding the method, now suddenly has to support more types for that parameter, and that is what's technically breaking here.

@GrahamCampbell
Copy link
Member

Those sort of breaking changes are common place in laravel's patch releases though, so I wouldn't bother with reverting or retargetting 5.6, unless someone is actually burnt bad by this.

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.

5 participants