Skip to content
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

Drop Illuminate/Support requirement #5

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

JayBizzle
Copy link
Contributor

Pretty sure you don't need to depend on illuminate/support as none of the code in the package uses it directly and the dependency is enforce by the parent lord/laroute package 👍

@forxer forxer merged commit 55fea62 into AXN-Informatique:master Aug 25, 2020
@forxer
Copy link
Member

forxer commented Aug 25, 2020

👍

@sergejostir
Copy link

Well, it is used now

return Arr::only($data, ['uri', 'name']);

so this should be reverted I guess.

@JayBizzle
Copy link
Contributor Author

JayBizzle commented Aug 25, 2020

Yes, we should revert 3582aef

JayBizzle referenced this pull request Aug 25, 2020
@sergejostir
Copy link

What? Then the new dependency would be laravel/helpers.

@forxer
Copy link
Member

forxer commented Aug 26, 2020

Pretty sure you don't need to depend on illuminate/support as none of the code in the package uses it directly and the dependency is enforce by the parent lord/laroute package 👍

This is fine, since lord/laroute require illuminate/support we don't need to require it in this package.

@JayBizzle
Copy link
Contributor Author

Ha, totally forgot about that 😂👍

@sergejostir
Copy link

Pretty sure you don't need to depend on illuminate/support as none of the code in the package uses it directly and the dependency is enforce by the parent lord/laroute package 👍

This is fine, since lord/laroute require illuminate/support we don't need to require it in this package.

Well, that is kinda bad practice. The package should clearly state all its requirements. Hypotetically lord/laroute can drop illuminate/support as its requirement at any time and as a result also breaking your package.
Or let's say, someone creates a replacement package (using composer's "replace" functionality) to lord/laroute without using illuminate/support and again your package will then not work.

@forxer
Copy link
Member

forxer commented Aug 26, 2020

@sergiz I quite agree with you in theory but less in practice.

The idea behind this change was that you could continue to use this package transparently without our intervention when a new release of Laravel occurs and that the package we are extending (lord/laroute) controls dependencies.

But ok I will put back the prerequisites to make it "cleaner" and in this case we also have to add the laravel/routing dependency which is not the case from the beginning.

@sergejostir
Copy link

I understand. Well, if you do not care about something breaking it, an option is also to specify dependency as ">=5.4".

@forxer
Copy link
Member

forxer commented Aug 26, 2020

Ok we have decided to release a new 1.7.0 version with this compromise.

(Otherwise yes: we know that we are not following SEMVER; it will be for a future 2.0)

Thank you all

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

Successfully merging this pull request may close these issues.

3 participants