Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

Add illuminate/contracts dependency #7

Conversation

X-Coder264
Copy link

The illuminate/contracts dependency was missing in composer.json and I refactored some config calls to make the dependency more obvious.

Copy link

@jstoone jstoone left a comment

Choose a reason for hiding this comment

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

For me personally I like the added typehinting, but as far as I remember all of the required illuminate/* packages have a direct dependency on illuminate/contracts themselves.

I'm not sure what the "best practises" are for this, but I think it's alright to rely on their dependency of illuminate/contracts.

@@ -29,6 +29,7 @@
"guzzlehttp/psr7": "^1.5",
"illuminate/broadcasting": "5.7.*",
"illuminate/console": "5.7.*",
"illuminate/contracts": "5.7.*",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"illuminate/contracts": "5.7.*",

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what the "best practises" are for this, but I think it's alright to rely on their dependency of illuminate/contracts.

We should not care about other packages dependencies. Each package should declare what dependencies it relies upon. If the dependencies this package depends upon suddenly drop their dependency on illuminate/contracts (which has a really low probability but still...) this package would also break which of course shouldn't happen.

Copy link

Choose a reason for hiding this comment

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

That does make sense. Thank you for taking the time to clarify. ✌️

@freekmurze
Copy link
Contributor

Thanks for your work on this but I'm gonna pass. I think using config() is just fine in this context.

@freekmurze freekmurze closed this Dec 5, 2018
@X-Coder264 X-Coder264 deleted the add-illuminate-contracts-dependency branch December 5, 2018 12:28
@X-Coder264
Copy link
Author

X-Coder264 commented Dec 5, 2018

@freekmurze Hm, I disagree. First, this PR is more about adding the illuminate/contracts dependency. Second, the config helper (and all other global helpers like app) are in the Foundation namespace which isn't there unless you require the entire laravel/framework package (which this package doesn't so using it is not "fine"). In order to lessen the coupling to the entire framework the solution is to use either the facades as they are in the illuminate/support package that this package already depends on or to just use dependency injection using the contracts provided by illuminate/contracts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants