Add support for Laravel 9.x

[marcroberts]

No description provided.

[marcroberts]

Now that Laravel 9 has been released al the tests related to this change are now passing - https://github.com/marcroberts/twilio/runs/5019943008

[L3o-pold]

With the introduction of this PR and this change

this part can't work and throw an exception:

#0 /var/www/html/vendor/laravel-notification-channels/twilio/src/TwilioChannel.php(89): App\\Models\\Registration->routeNotificationFor('NotificationCha...', Object(App\\Notifications\\Register))
#1 /var/www/html/vendor/laravel-notification-channels/twilio/src/TwilioChannel.php(47): NotificationChannels\\Twilio\\TwilioChannel->getTo(Object(App\\Models\\Registration), Object(App\\Notifications\\Register))
#2 /var/www/html/vendor/laravel/framework/src/Illuminate/Notifications/NotificationSender.php(148): NotificationChannels\\Twilio\\TwilioChannel->send(Object(App\\Models\\Registration), Object(App\\Notifications\\Register))
#3 /var/www/html/vendor/laravel/framework/src/Illuminate/Notifications/NotificationSender.php(106): Illuminate\\Notifications\\NotificationSender->sendToNotifiable(Object(App\\Models\\Registration), '4a788d2a-df79-4...', Object(App\\Notifications\\Register), 'NotificationCha...')

[marcroberts]

Hmm

Looking back at the changes to that file, the comments indicate that the routeNotificationsFor() method in Laravel has been expecting the $driver parameter to be a string since at least as far back as Laravel 5.5

Shall we remove

if ($notifiable->routeNotificationFor(self::class, $notification))
    return $notifiable->routeNotificationFor(self::class, $notification);
}

And rely on the string call afterwards?

if ($notifiable->routeNotificationFor('twilio', $notification)) {
    return $notifiable->routeNotificationFor('twilio', $notification);
}

[L3o-pold]

We should wait for laravel/framework#40880

[L3o-pold]

Hmm

Looking back at the changes to that file, the comments indicate that the routeNotificationsFor() method in Laravel has been expecting the $driver parameter to be a string since at least as far back as Laravel 5.5

Shall we remove

if ($notifiable->routeNotificationFor(self::class, $notification))
    return $notifiable->routeNotificationFor(self::class, $notification);
}

And rely on the string call afterwards?

if ($notifiable->routeNotificationFor('twilio', $notification)) {
    return $notifiable->routeNotificationFor('twilio', $notification);
}

self::class is a string

[marcroberts]

self::class is a string

Of course it is 🤦‍♂️

I'll revert that commit, and add a test to cover this issue

[marcroberts]

I'm not sure we need to test actually. If we're going to wait for the fix upstream we can just change the dependency to be ^9.0.1 (or whatever the release is) instead.

If wanted, the failure can be reproduced in the tests by removing the empty routeNotificationsFor method in the Notifiable class within TwilioChannelTest and replacing it was use RoutesNotifications;

[marcroberts]

Laravel 9.0.1 has just been released which includes laravel/framework#40880

I've also removed the Laravel 6 & PHP 8.1 combination from the actions workflow (This seemed to pass before but Laravel 6 does not support PHP 8.1).

[onlime]

@marcroberts thanks for your PR. Anything holding you back from merging it? Who is the maintainer of this package?

I have tested it and didn't run into any issues. Installed it in a Laravel 9 project using the following in composer.json:

    "repositories": [
        {
            "type": "git",
            "url": "https://github.com/marcroberts/twilio.git"
        }
    ],
    "require": {
        "laravel-notification-channels/twilio": "dev-laravel9"
    },
    "minimum-stability": "dev",
    "prefer-stable": true

[miken32]

Last couple of PRs were approved by @atymic but the last one was a year ago...