anlutro / php-bulk-sms

BulkSMS API - PHP implementation
MIT License
36 stars 27 forks source link

Change calling share() to singleton() in register method #13

Closed peZhmanParsaee closed 7 years ago

anlutro commented 7 years ago

Hey, thanks for the PR. I'm interested in keeping backwards compatibility if possible, so have a couple of questions.

What versions of Laravel does share no longer work with?

Does singleton work just like share for older Laravel versions?

peZhmanParsaee commented 7 years ago

Hey, my pleasure.

As of laravel 5.4 share has been removed. You will have to use the singleton instead.

See: https://github.com/laravel/framework/commit/1a1969b6e6f793c3b2a479362641487ee9cbf736

Reference: https://laracasts.com/discuss/channels/laravel/undefined-method-illuminatefoundationapplicationshare-when-upgrading-to-laravel-54

Regards

anlutro commented 7 years ago

I'd like to keep backwards compatibility which I'm not sure changing share to singleton will do, and I don't have time to test it properly. I pushed an alternative fix for the problem to the master branch just now, can you try updating the package to version dev-master@dev to see if it works?

stojankukrika commented 7 years ago

@anlutro it's works like that... TNX!

peZhmanParsaee commented 7 years ago

I'd love to test it, but actually I'm new to laravel.

I saw your changing code , it is this obviously:

    if (version_compare(Application::VERSION, '5.4', '>=')) {
        $this->app->singleton('bulksms', $factory);
    } else {
        $this->app['bulksms'] = $this->app->share($factory);
    }

I didn't test it, but I think it is correct, because I saw laravel 5.4 source code: https://github.com/laravel/framework/releases/tag/v5.4.0

And also I saw laravel version 5.3.29 source code ( that is immediately before version 5.4.0 ): https://github.com/laravel/framework/releases/tag/v5.3.29

The share() method has been removed since version 5.4.0 and it is'nt exists in version 5.4.0, but it is exists in version 5.3.29.

Thank you 👍

anlutro commented 7 years ago

@peZhmanP you can test it by running composer require anlutro/bulk-sms:dev-master@dev in you project.