-
Notifications
You must be signed in to change notification settings - Fork 97
Created a provider to inject the session object into Elogram's container #25
Conversation
Thanks, I'll take a look in this as soon as I've time to spare. |
$container = $this->getContainer(); | ||
|
||
$container->share(SessionStore::class, function () use ($config) { | ||
$app = Application::getInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that isn't correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no reason to assume someone is actually using laravel, so they might not have facades booted, or even have an application class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a package to bridge Laravel with my package, why wouldn't you assume they're using Laravel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a package to bridge Laravel with my package, why wouldn't you assume they're using Laravel?
Why should you assume that? It's a bridge package to anyone using laravel COMPONENTS. This could include the lumen framework. This package DOES NOT depend on laravel/framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Laravel Instagram is an Instagram bridge for Laravel and Lumen.
Didn't see "components" anywhere. If you're using plain Laravel components, why even bother using this package? Why not use the Instagram one directly? If you're using Laravel/Lumen, then use this? You do make a valid point about Lumen support, though.
Please merge this |
This repo is just useless until this is merged, isn't it? |
I'll take a look at this soon. |
Is this fixed and merged yet? Need this library. |
Green = not merged |
That change I pointed out still needs fixing. |
Closing this in favour of larabros/elogram#40 |
No description provided.