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

Laravel 6 #26

Closed
ultrono opened this issue Sep 3, 2019 · 11 comments
Closed

Laravel 6 #26

ultrono opened this issue Sep 3, 2019 · 11 comments

Comments

@ultrono
Copy link

ultrono commented Sep 3, 2019

Are you planning to support Laravel 6?

@weotch
Copy link
Member

weotch commented Sep 3, 2019

Probably not in the short term but happy to take PRs or add a new maintainer if someone wants to contribute.

@djoudi
Copy link
Contributor

djoudi commented Sep 5, 2019

i try to upgrade it :)

@ihorvorotnov
Copy link

I just bumped version constraints in both cloner and upchuck packages, pointed composer to my forks and was able to upgrade to Laravel 6. However, some updates in tests were required as well.

I use cloner in Laravel Nova to handle resource cloning, tested here and there - seems to work perfectly fine with L6.

@ultrono
Copy link
Author

ultrono commented Sep 18, 2019

I just bumped version constraints in both cloner and upchuck packages, pointed composer to my forks and was able to upgrade to Laravel 6. However, some updates in tests were required as well.

I use cloner in Laravel Nova to handle resource cloning, tested here and there - seems to work perfectly fine with L6.

Spot on, I'm looking at https://github.com/ihorvorotnov/cloner/commits/master.

it may be worth specifying versions restraints like https://github.com/owen-it/laravel-auditing/blob/master/composer.json#L42-L46 - this way you can allow version 5.8 and 6.0.

It a shame that forks are required for smaller packages like this, as Laravel 6 is a LTS release.

I guess for the time being forks are required :(

@ihorvorotnov
Copy link

ihorvorotnov commented Sep 18, 2019

@ultrono Yeah, I used most recent versions just to test whether it'll work with L6 out of the box, without any actual code changes. Went through L6 upgrade guide and checked for usage of changed methods etc - didn't find anything within the package code so I'm assuming it's a matter of versions bump only.

Giving it another day or two for my editors to test the cloning functionality on a real project, if anything goes wrong - they'll let me know. If not - will update versions with alternatives and submit PRs to both packages.

Worth mentioning that tests are actually broken (apart from using long deprecated PHPUnit_Framework_TestCase), but there's a separate issue/PR trying to address this so I'll probably skip it in my PRs.

Also, one more thing - the package uses (outdated) satooshi/php-coveralls which is deprecated and should be replaced with php-coveralls/php-coveralls. However, that should be handled in a separate PR too I believe.

@djoudi
Copy link
Contributor

djoudi commented Sep 18, 2019

#27

@ultrono
Copy link
Author

ultrono commented Sep 19, 2019

@ultrono Yeah, I used most recent versions just to test whether it'll work with L6 out of the box, without any actual code changes. Went through L6 upgrade guide and checked for usage of changed methods etc - didn't find anything within the package code so I'm assuming it's a matter of versions bump only.

Giving it another day or two for my editors to test the cloning functionality on a real project, if anything goes wrong - they'll let me know. If not - will update versions with alternatives and submit PRs to both packages.

Worth mentioning that tests are actually broken (apart from using long deprecated PHPUnit_Framework_TestCase), but there's a separate issue/PR trying to address this so I'll probably skip it in my PRs.

Also, one more thing - the package uses (outdated) satooshi/php-coveralls which is deprecated and should be replaced with php-coveralls/php-coveralls. However, that should be handled in a separate PR too I believe.

Great stuff. Have just pulled down your updates into a vanilla Laravel 6 project. Cloning simple models and models with multiple relations worked great 👍

@weotch
Copy link
Member

weotch commented Sep 20, 2019

I released this as 3.5.0, thanks @djoudi.

Also, if anyone wants to get involved as a maintainer, lemme know and I'll add you to the project.

@weotch weotch closed this as completed Sep 20, 2019
@denisdulici
Copy link
Collaborator

@weotch it seems you forgot to merge the pr #27

@ultrono
Copy link
Author

ultrono commented Sep 21, 2019

Actually, not sure of it's just myself but the changes aren't present on packagist or the main repository i.e. https://github.com/BKWLD/cloner/blob/master/composer.json#L13 vs Laravel 6 support from #27 at

"illuminate/support": "^5.5|^6.0"

@djoudi
Copy link
Contributor

djoudi commented Sep 21, 2019

i don't see the change at 3.5.0 release

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

No branches or pull requests

5 participants