anlutro / laravel-settings

Persistent settings in Laravel
MIT License
940 stars 112 forks source link

Deferred service provider #154

Closed sausin closed 3 years ago

sausin commented 3 years ago

Closes #150

sausin commented 3 years ago

@bweston92 That can definitely be an option. We will either have a new service provider for the interface. But then, for those that are manually importing the service provider in their projects (>=5.5) a different ServiceProvider file would be a breaking change. If we have a new file for the older versions, then the package will be break for older laravel versions.

@anlutro it would be great to get your thoughts as well !

bweston92 commented 3 years ago

@sausin how are consumers using this without the deferrable interface on the service provider?

If we want to keep it to the same class name, we would need to create a base class and then has the following for the service provider.

if (interface_exists('Illuminate\Contracts\Support\DeferrableProvider'))
  class ServiceProvider extends BaseServiceProvider implements \Illuminate\Contracts\Support\DeferrableProvider {}
else
  class ServiceProvider extends BaseServiceProvider {}
bweston92 commented 3 years ago

@sausin any update, I can take over if you don't have the time to progress on this PR.

sausin commented 3 years ago

@sausin how are consumers using this without the deferrable interface on the service provider?

If we want to keep it to the same class name, we would need to create a base class and then has the following for the service provider.

if (interface_exists('Illuminate\Contracts\Support\DeferrableProvider'))
  class ServiceProvider extends BaseServiceProvider implements \Illuminate\Contracts\Support\DeferrableProvider {}
else
  class ServiceProvider extends BaseServiceProvider {}

Apologies for the delay in getting back. Without the DeferableProvider there is no difference wrt usage, it's only about performance.

Did you mean to suggest that we will basically move the interface logic to a BaseServiceProvider which will keep the ServiceProvider class clean in a way?

I'll was going to update this PR on the weekend. If you're okay to take it on before then please feel free do so. :+1:

bweston92 commented 3 years ago

Ok so there is no break changes, it would mean the consumers would need to just update to new version and use the DeferrableServiceProvider.

So if you leave the service provider as was and do class DeferrableServiceProvider extends ServiceProvider implements \Illuminate\Contracts\Support\DeferrableProvider I think that would be the best way, and update the documentation to use this service provider when using Laravel 5.8 or above.

Thanks, have a good weekend.

sausin commented 3 years ago

@bweston92 I've updated the PR with hopefully agreeable changes. Edits were made within Github's editor so I hope no typo's got left behind.

bweston92 commented 3 years ago

Not quite what I was saying, but it should work. I will merge and do a test run when I can. Thanks for your contribution @sausin