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] Allow MailFake to assert mailables were queued #20454

Merged
merged 16 commits into from
Aug 17, 2017

Conversation

mateusjatenee
Copy link
Contributor

@mateusjatenee mateusjatenee commented Aug 7, 2017

Hey everyone,

Right now when you queue a mailable you have to either assert it was sent (it was not, as it was queued) or you have to assert Illuminate\Mail\SendQueuedMailable was pushed on to the queue.

This PR gives queued mailables their own property and allows assertions to be made, such as assertWasQueued($mailable).

Let me know what you guys think!

* @return \Illuminate\Support\Collection
*/
protected function mailablesOf($type)
protected function mailablesOf($type, $queued = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just my two cents but I'd make this a separate method. It would only be duplicating three lines and a method called queuedMailablesOf makes the intent much clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, thank you!

Added a commit that changes thi

@mateusjatenee
Copy link
Contributor Author

Not entirely related, but perhaps we could build the view so the user knows that there are syntax errors or any other kind of error. I opened an issue in laravel/internals laravel/ideas#720

@taylorotwell
Copy link
Member

So would this be a breaking change? assertSent would no longer work for them?

@taylorotwell
Copy link
Member

@mateusjatenee building the view is a separate test IMO. and one that can already be written pretty easily.

@taylorotwell
Copy link
Member

This also doesn't work correctly if a Mailable is implementing ShouldQueue and is sent using Mail::send()

@mateusjatenee
Copy link
Contributor Author

@taylorotwell Sorry I forgot to queue it if It implemented ShouldQueue — I did this late and forgot it.

I added some code that verifies if the mailable implements the interface and queues if it does. Given #20485 was approved I also added the option to specify how many times it should have been queued.

It would be a BC, yes.. but I believe it's worth it as the "migration" is very simple and verifying a mailable was queued with sent doesn't really make sense I guess.

@taylorotwell taylorotwell merged commit b24f752 into laravel:master Aug 17, 2017
@taylorotwell
Copy link
Member

Thanks

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.

3 participants