search

hootlex / laravel-friendships

This package gives Eloquent models the ability to manage their friendships.
MIT License
703 stars 151 forks source link

Support for laravel 5.8 #124

Open moecasts opened 5 years ago

moecasts commented 5 years ago

The fire method (which was deprecated in Laravel 5.4) of the Illuminate/Events/Dispatcher class has been removed. You should use the dispatch method instead.

moecasts commented 5 years ago

@hootlex Server Requirements The Laravel framework has a few system requirements. All of these requirements are satisfied by the Laravel Homestead virtual machine, so it's highly recommended that you use Homestead as your local Laravel development environment.

However, if you are not using Homestead, you will need to make sure your server meets the following requirements:

PHP >= 7.1.3 OpenSSL PHP Extension PDO PHP Extension Mbstring PHP Extension Tokenizer PHP Extension XML PHP Extension Ctype PHP Extension JSON PHP Extension BCMath PHP Extension

Travis CI only test by php 7.0.25, so it failed.

hootlex commented 5 years ago

@MoeCasts are you planning to finish this PR?

moecasts commented 5 years ago

@hootlex I have try my best to make it work with laravel 5.8. It works in php 7.2 now.

GreenImp commented 5 years ago

Am I right in thinking that the only reason this is failing is because the tests are also being run against PHP 5.6 and 7? Laravel >= 5.6 only supports PHP >= 7.1.3.

So, if this plugin needs to work in the lastest versions of Laravel, then surely the PHP version needs to be upped? You'd then be in a situation where people using Laravel < 5.6 would have to stick to this package version before this was released, but I've seen other packages do that.

moecasts commented 5 years ago

@GreenImp The pr only replace Event::fire to Event::dispatch. If you read the Event::fire method, you will find that Event::fire is a alias of Event::dispatch. So, it will work though using laravel <= 5.8.

GreenImp commented 5 years ago

So it's the addition of "beyondcode/laravel-dump-server": "^1.2", in the composer.js file, because that requires PHP >= 7.1? Is that required for Laravel 5.8 (I'm assuming so)? If so, it may be worth either dropping support for older versions of PHP, or drop compatibility with Laravel < 5.6 in future releases.

I realise that you're not the project owner here, I'm just voicing my thoughts. Maybe @hootlex can say whether this is something to do or not. I'm not sure how it can support Laravel >= 5.8 otherwise?

moecasts commented 5 years ago

@GreenImp If you wanna run on Laravel >= 5.4, you only need to replace Event::fire to Event::dispatch. The other changes are added to pass test. I don't know php unit test, so I can not say it.

kainxspirits commented 5 years ago

@hootlex @MoeCasts PHP 5.6 and 7.0 support should be dropped (only changing the yaml file should be fine ?). Starting Laravel 5.6, the minimum PHP version required was 7.1.3.

I also think the laravel version in the composer.json file should be explicitly defined. Only having 5.* is not really a good idea.

tquiroga commented 5 years ago

When will this be merged?