felixkiss / uniquewith-validator

Custom Laravel Validator for combined unique indexes
MIT License
389 stars 129 forks source link

Add support for Laravel 6 #107

Closed erikgaal closed 5 years ago

erikgaal commented 5 years ago

Closes #110

This PR adds support for the upcoming Laravel 6.0 release. We had to upgrade to phpspec 5 for development, because illuminate/support@^6.0 requires symfony/process@^4.2.

felixkiss commented 5 years ago

I like it, but if we drop support for older PHP/Laravel versions we would need to release a new major version of the package.

That would mean everyone needs to go through the upgrade process or not ger future updates.

erikgaal commented 5 years ago

@felixkiss I see, however, bugfix support for Laravel 5.4 has ended over 2 years ago. I think that supporting from Laravel 5.5 and upwards is easier, especially given that it allows us to bump the PHP version requirement to >=7.

felixkiss commented 5 years ago

Maybe now with Laravel 6 released it is a good time to release a new major version of this package as well. I’m guessing people will be checking many packages during the upgrade process.

What would you do?

erikgaal commented 5 years ago

I think dropping support for <5.5 is mandatory for the health of this project. But then there are two options regarding versions up until 5.8.

  1. Mark a new major version with support for only ^6.0, or
  2. Mark a new major version with support for >=5.5.* and ^6.0

I think keeping support for lower versions is fair for the community and I personally don't think supporting them for now is that much work. So I would go for option 2.

erikgaal commented 5 years ago

Right, so we have one last issue. The \Illuminate\Contracts\Translation\Translator interface was changed in https://github.com/laravel/framework/pull/29529. It was changed from Translator::trans() to Translator::get(). This causes the build for Laravel 6 to fail. They mention that the helper function __() was not changed. How should we adapt this change in this package, @felixkiss?

felixkiss commented 5 years ago

Yeah, I noticed that yesterday on the failing Travis builds. Is the helper function part of one of the packages we require?

If not, I could also see us releasing version 4.0 as Laravel 6 only. That would certainly make maintaining life easier.

erikgaal commented 5 years ago

The trans() helper is only available in the laravel/framework package. Maybe we could investigate how another rule package would solve this? I see that https://github.com/spatie/laravel-validation-rules uses Rule objects instead of extensions, but that would still result in a breaking change.

felixkiss commented 5 years ago

Either check if method exists like I did here or only support Laravel 6.0 in the new version.

erikgaal commented 5 years ago

@felixkiss I've used method_exists so we can create a minor release that adds support for ^6.0.

felixkiss commented 5 years ago

Thanks for the work @erikgaal! I think I can look at this tomorrow.

erikgaal commented 5 years ago

@felixkiss You could release this as a new minor version, as it is backwards compatible and the version constraint still allows for Laravel versions 5.*. I think we should clean up and remove the version dependent code in the future and release a new major version with support for only ^6.0.

felixkiss commented 5 years ago

Thanks @erikgaal!