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

Support for laravel 5.8 #124

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Support for laravel 5.8 #124

wants to merge 11 commits into from

Conversation

moecasts
Copy link

@moecasts moecasts commented Mar 7, 2019

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.

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
Copy link
Author

moecasts commented Mar 7, 2019

@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
Copy link
Owner

hootlex commented Apr 9, 2019

@moecasts are you planning to finish this PR?

@moecasts
Copy link
Author

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

@GreenImp
Copy link

GreenImp commented May 14, 2019

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
Copy link
Author

@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
Copy link

GreenImp commented May 16, 2019

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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link

When will this be merged?

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.

5 participants