Skip to content

Commit

Permalink
Move illuminate dependency to require-dev
Browse files Browse the repository at this point in the history
  • Loading branch information
@driesvints
driesvints committed Dec 4, 2018
1 parent 2ef2a80 commit 03edd42
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions composer.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
],
"require": {
"php": "^7.1.3",
"nexmo/client": "^1.0",
"illuminate/notifications": "~5.7"
"nexmo/client": "^1.0"
},
"require-dev": {
"illuminate/notifications": "~5.7",
"mockery/mockery": "^1.0",
"phpunit/phpunit": "^7.0"
},
Expand Down

9 comments on commit 03edd42

@AbdelElrafa
Copy link

Choose a reason for hiding this comment

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

@driesvints @themsaid Why was illuminate/notifications moved out of the require? Isn't it necessary to run the package?

@driesvints
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but you're never going to use this outside laravel/framework anyway which will include it. We removed it because it was causing a cyclic dependency.

@AbdelElrafa
Copy link

Choose a reason for hiding this comment

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

This is a dependency of the package in order to get it to work. Moving it to the dev doesn't change anything. The only things in the dev should be the packages used for dev/testing exclusively. Any package used by the code is a requirement.

@AbdelElrafa
Copy link

Choose a reason for hiding this comment

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

@driesvints @themsaid
Here is the dependency flow currently:

My Laravel App => requires [
    laravel/framework => requires [
        illuminate/notifications,
        laravel/nexmo-notification-channel => requires [
            illuminate/notifications,
        ]
    ]
]

This is not a cyclic dependency. A cyclic dependency would be if illuminate/notifications required laravel/nexmo-notification-channel. Composer doesn't have issues with cyclic dependencies from what I found in my research, and moving it to the dev section wouldn't solve the issue if there was one.

@driesvints
Copy link
Member Author

Choose a reason for hiding this comment

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

@AbdelElrafa a PR got merged which does exactly this to maintain BC for people who require illuminate/notifications separately: laravel/framework#26727

@AbdelElrafa
Copy link

Choose a reason for hiding this comment

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

Got it, thank you.

@driesvints
Copy link
Member Author

Choose a reason for hiding this comment

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

No problem! Appreciate it that you thought this through :)

@driesvints
Copy link
Member Author

Choose a reason for hiding this comment

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

@AbdelElrafa if we remove them on the 5.7 branch they won't be included anymore in the https://github.com/illuminate/notifications subsplit and everyone who currently requires the 5.7.* version of that subsplit and relies on the slack and nexmo channels will experience a breaking change. The packages themselves are removed again on the master branch of laravel/framework like you suggest.

@AbdelElrafa
Copy link

Choose a reason for hiding this comment

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

@driesvints 🤦‍♂️ I understand now... Although this might be a good argument as to why changing this in 5.7 is a breaking change.

Please sign in to comment.