laravel-notification-channels / twilio

Twilio notifications channel for Laravel
https://laravel-notification-channels.com
227 stars 36 forks source link

Add/improve tests, improve testability and refactor #17

Closed casperboone closed 8 years ago

casperboone commented 8 years ago

As this is currently the most downloaded package, I thought the tests needed some attention to prove the working to its users.

Note: This PR proposes something a little different than #14. Pick what you like most ;).

I have done the following:

casperboone commented 8 years ago

Since this PR is changing quite a lot, please try it out in an actual Laravel application before releasing it. I'm not a Twilio user myself, so I'm not able to actually use it. Please let me know if anything goes wrong, so I can fix it ;).

I prefer this PR over #14, since this one made the tests, but also the code in general a lot cleaner and more readable. However, it does introduce some changes you might not like. So, pick the one you like most ;).

gregoriohc commented 8 years ago

Hi @casperboone,

I prefer this PR to the other one. There is only one problem. When I try to make real test, it gives me the following error:

Illuminate\Contracts\Container\BindingResolutionException with message 'Unresolvable dependency resolving [Parameter #1 [ $from ]] in class NotificationChannels\Twilio\Twilio'

The problem is in the bindings. When you call $this->app->make(TwilioService::class) in the Service Provider the container doesn't know that the calling class is \NotificationChannels\Twilio\Twilio, so it doesn't resolve it.

I have found two possible solutions on the Service Provider boot method. The first one would look like this:

$this->app->when(TwilioChannel::class)
    ->needs(Twilio::class)
    ->give(function () {
        $config = $this->app['config']['services.twilio'];

        return new Twilio(
            $this->app->make(TwilioService::class, [
                $config['account_sid'],
                $config['account_sid'],
            ]),
            $config['from']
        );
    });

And the other option would look like this:

$this->app->when(Twilio::class)
    ->needs(TwilioService::class)
    ->give(function () {
        $config = $this->app['config']['services.twilio'];

        return new TwilioService(
            $config['account_sid'],
            $config['auth_token']
        );
    });

$this->app->when(Twilio::class)
    ->needs('$from')
    ->give($this->app['config']['services.twilio.from']);

Which one do you prefer? Can you fix it and pull the commit? Thanks!!

casperboone commented 8 years ago

Fixed it, thanks! :)