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.7] Extract Nexmo and Slack drivers to own package #26689

Merged
merged 3 commits into from Dec 3, 2018

Conversation

Projects
None yet
10 participants

themsaid added some commits Nov 30, 2018

@koenhoeijmakers

This comment has been minimized.

koenhoeijmakers commented Nov 30, 2018

Why not in https://github.com/laravel-notification-channels as the docs still reference to it as a sort of "collection of all notification channels"?

@devcircus

This comment has been minimized.

Contributor

devcircus commented Nov 30, 2018

That’s a “community” driven repo. These are official Laravel packages.

@koenhoeijmakers

This comment has been minimized.

koenhoeijmakers commented Nov 30, 2018

Can totally understand that.

It's just that laravel-notification-channels doesn't seem that active in the form of adding new channels as the oldest new channel PR dates from Sep 6, 2016 and that this might have been a point to pick that up again.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 30, 2018

We need to add them to the framework composer.json I guess? And then remove them in 5.8 and people opt-in.

@laurencei

This comment has been minimized.

Member

laurencei commented Nov 30, 2018

Seems very strange to be doing this on a point release (even if its not breaking). Why not leave the whole change until 5.8 entirely?

@themsaid

This comment has been minimized.

Member

themsaid commented Dec 1, 2018

@taylorotwell added to the framework's composer.json file.

@laurencei

This comment has been minimized.

Member

laurencei commented Dec 1, 2018

@themsaid - Travis is failing with "Your requirements could not be resolved to an installable set of packages"

@themsaid

This comment has been minimized.

Member

themsaid commented Dec 1, 2018

@laurencei yes, for now. We'll need to publish the libraries on packagist first.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Dec 1, 2018

@themsaid Are we going to move the tests to the repos?

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Dec 1, 2018

Both packages have been added to Composer.

@themsaid

This comment has been minimized.

Member

themsaid commented Dec 1, 2018

@taylorotwell build is passing now. Also added tests to the packages.

@mfn

This comment has been minimized.

Contributor

mfn commented Dec 1, 2018

Seems very strange to be doing this on a point release (even if its not breaking). Why not leave the whole change until 5.8 entirely?

My thoughts exactly. What's the rush here, really? Just do it in-time for master.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Dec 2, 2018

@mfn @laurencei Because I want to do it now? Do you have a reason why we shouldn't? It's not a breaking change in any way unless I'm mistaken?

@laurencei

This comment has been minimized.

Member

laurencei commented Dec 2, 2018

It felt strange initially - and then I considered the fact that the framework is essentially a bunch of packages, and so in that respect it makes sense. I agree there is no breaking change that I can think of either.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Dec 3, 2018

@themsaid do you want to tag 1.0.0 on both of the packages?

@taylorotwell taylorotwell merged commit 57dedf1 into laravel:5.7 Dec 3, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TBlindaruk

This comment has been minimized.

Member

TBlindaruk commented Dec 3, 2018

@driesvints

This comment has been minimized.

Member

driesvints commented Dec 3, 2018

@TBlindaruk it's a bit to late for that since we already tagged versions. Didn't know about this. Thanks for sharing! :)

@TBlindaruk

This comment has been minimized.

Member

TBlindaruk commented Dec 3, 2018

@driesvints we still can do it )

Like:

  1. delete files from the package
  2. moved files with the history

;)

@stevebauman

This comment has been minimized.

stevebauman commented Dec 7, 2018

This seems to have broken my Laravel 5.7 app.

Downgrading to Laravel 5.7.15 works without issue, but as soon as I upgrade to Laravel 5.7.16 it breaks:

As soon as I run composer update targeting 5.7.16, this happens:

Package operations: 13 installs, 1 update, 0 removals
  - Installing guzzlehttp/promises (v1.3.1): Loading from cache
  - Installing ralouphie/getallheaders (2.0.5): Loading from cache
  - Installing psr/http-message (1.0.1): Loading from cache
  - Installing guzzlehttp/psr7 (1.5.2): Loading from cache
  - Installing guzzlehttp/guzzle (6.3.3): Loading from cache
  - Installing laravel/slack-notification-channel (v1.0.2): Loading from cache
  - Updating laravel/framework (v5.7.15 => v5.7.16): Loading from cache
  - Installing lcobucci/jwt (3.2.5): Loading from cache
  - Installing php-http/promise (v1.0.0): Loading from cache
  - Installing php-http/httplug (v1.1.0): Loading from cache
  - Installing php-http/guzzle6-adapter (v1.1.1): Loading from cache
  - Installing zendframework/zend-diactoros (1.8.6): Loading from cache
  - Installing nexmo/client (1.5.2): Loading from cache
  - Installing laravel/nexmo-notification-channel (v1.0.1): Loading from cache
lcobucci/jwt suggests installing mdanter/ecc (Required to use Elliptic Curves based algorithms.)
Writing lock file
Generating autoload files
> Illuminate\Foundation\ComposerScripts::postAutoloadDump
> @php artisan package:discover


In Container.php line 960:

  Unresolvable dependency resolving [Parameter #0 [ <required> $app ]] in class Illuminate\Support\Manager

Exception:

error

@driesvints

This comment has been minimized.

Member

driesvints commented Dec 7, 2018

@stevebauman

This comment has been minimized.

stevebauman commented Dec 7, 2018

Thanks @driesvints 😄, that was indeed the issue. To resolve I had to manually delete app/bootstrap/cache/config.php as artisan wouldn't boot properly with this issue so couldn't use php artisan config:clear.

@mfn

This comment has been minimized.

Contributor

mfn commented Dec 9, 2018

Really, this should not have landed in a minor version :|

See #26689 (comment) and #26689 (comment)

@fitztrev

This comment has been minimized.

Contributor

fitztrev commented Dec 10, 2018

@mfn It could be a breaking change. Now, the nexmo/client package and all its dependencies are installed in my app, when before it was just a "suggested" package left for me to decide if I want to use that driver. The nexmo/client package has a number of outdated dependencies:

Package                      Installed  Latest   Description
============================================================================
php-http/guzzle6-adapter     v1.1.1     v2.0.0   Guzzle 6 HTTP Adapter
php-http/httplug             v1.1.0     v2.0.0   HTTPlug, the HTTP client abstraction for PHP
ralouphie/getallheaders       2.0.5      3.0.1   A polyfill for getallheaders.
zendframework/zend-diactoros  1.8.6      2.0.1   PSR HTTP Message implementations

I don't already use any of those but if I did, and had the latest version of any of them installed, it would cause issues.

I noticed on the master branch these have been removed: 88b9fd6

@TBlindaruk

This comment has been minimized.

Member

TBlindaruk commented Dec 10, 2018

@driesvints

This comment has been minimized.

Member

driesvints commented Dec 10, 2018

@fitztrev At the moment we're not getting any issues about this so I wouldn't revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment