FriendsOfSylius / SyliusImportExportPlugin

Sylius plugin to import / export data
MIT License
118 stars 82 forks source link

Update bundle for Sylius 1.9 / Symfony 4.4+||5.2+ #251

Closed Gogopex closed 3 years ago

Gogopex commented 3 years ago
Q A
Bug fix? yes
New feature? no
BC breaks? no
Fixed tickets #241, update to Sylius 1.9

I was met with quite a few errors trying to install this plugin on a brand new Sylius 1.9 webapp. I decided to take some time to fix these, in case you guys want to update this bundle for Symfony 1.9

This PR includes a bunch of updates to a few files:

With just these changes, the bundle installs and all features are working fine. πŸ‘


Note: the builds fail because they are using the old dependencies:

DEPENDENCY_VERSIONS="sylius/sylius:1.6.* symfony/symfony:4.2.*"

They should pass once the depency versions are "sylius/sylius:1.9.* symfony/symfony:5.2.*" πŸ‘

lsmith77 commented 3 years ago

thank you!

do you also have time to look into the travis setup?

Gogopex commented 3 years ago

thank you!

do you also have time to look into the travis setup?

No problem πŸ‘ I can take a look at the travis setup too, I will update this PR once it's done !

lsmith77 commented 3 years ago

BTW most OSS projects have migrated to GithubActions due to Travis slowness. but then again maybe due to everyone having moved Travis is now acceptable in speed.

Gogopex commented 3 years ago

Ok, this first attempt with Travis did not go too well. I'm not super familiar with it but I'll attempt to run the pipeline locally if possible to tinker with it till it runs. πŸ‘

lsmith77 commented 3 years ago

I could imagine using composer 2 could solve the memory issue

Gogopex commented 3 years ago

I made some progress on having a passing travis job πŸ™

However, the builds are still failing. There is something obvious I must be missing.

Locally I've had no issues with the composer update and everything is working fine, but the Travis job keeps failing on different dependencies mismatches. Here's what I run locally to simulate the job's steps:

composer require "sylius/sylius:1.9.*" --no-update
composer config extra.symfony.require "^5.2"
composer update --prefer-dist

And it goes through without hiccups. Not the case in the Travis job though. ☹️

So I'll be using my fork for now, but I'll come back to this Travis job during the week. If anyone wants to give it a go, feel free !

Gogopex commented 3 years ago

Thank you, @oallain ! I decided to give up on fighting with Travis and started exactly what you are suggesting this morning: https://github.com/MyLittleParis/SyliusImportExportPlugin/pull/6/files

Work got in the way but once I get the github action working on the fork, I will include it in this PR !

I do have one question: what do you think about dropping the support for Sylius <1.7 in order to reduce the number of builds that would run?

Edit: github actions are currently down..! https://www.githubstatus.com/

oallain commented 3 years ago

Thank you, @oallain ! I decided to give up on fighting with Travis and started exactly what you are suggesting this morning: https://github.com/MyLittleParis/SyliusImportExportPlugin/pull/6/files

Work got in the way but once I get the github action working on the fork, I will include it in this PR !

So cool 😎

I do have one question: what do you think about dropping the support for Sylius <1.7 in order to reduce the number of builds that would run?

Yes, to drop Sylius 1.7 and less. But keep 1.8 version.

Edit: github actions are currently down..! https://www.githubstatus.com/

πŸ€•

lsmith77 commented 3 years ago

agreed for the bump to 1.8 .. and oh the irony of life with GA being down.

but super appreciative of your commitment and work here @Gogopex

Gogopex commented 3 years ago

Hi guys ! I've finally gotten around to finishing this up, here's what's new:

The pipeline is now fully working; however it now crashes on the Phpstan's step, which doesn't seem to have anything to do with this PR πŸ‘

@oallain @lsmith77 see https://github.com/FriendsOfSylius/SyliusImportExportPlugin/actions/runs/683847071 for where it fails

There is two solutions ahead imo:

I'm not sure I can fix all of these issues this week, but I might be able to in the next 10 days, if you guys are too busy. πŸ‘

lsmith77 commented 3 years ago

I think we should disable PHPStan in this PR and then have a follow up PR addressing this topic. This PR and you have been burdened so much already without a merge.

Will do a deeper review tomorrow.

Gogopex commented 3 years ago

Hello,

I've taken into account @mbabker's suggestion, thanks πŸ‘

Turns out I also had to comment out PHPSpec (link to example failed build) along with Behat (link to example failed build) for now, otherwise the builds would not pass (the equivalent builds were failing on Jenkins before this PR 😦).

I agree a second PR to fix PHPStan and PHPSpec would be great, along with a PR for the Behat tests! Then everything would be all tidied up!

lsmith77 commented 3 years ago

amazing work!

I have created follow up tickets for PHPStan and PHPSpec