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

Gogopex · 2021-03-06T23:01:32Z

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:

a bump to the symfonystopwatch version, allowing the bundle to install succesfully after being composer require'd
an update to src/DependencyInjection/Configuration.php which ensures that it both works on Symf 5.2, but also maintains BC with 4.1 and older
~~replaced the ObjectMapper typings with EntityManagerInterface~~
added : inttyping and return 0; to the execute() of all commands (required by Symf 4.4+. I did not use return Command::Success to maximize BC.)
updated factory declaration of sylius.class_metadata.product_image in services.yml to symf4.4+ format

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.*" 👍

…r with EntityManagerInterface to update bundle for sylius 1.9

Update bundle to make it work with Sylius 1.9

fixed returns

composer.json

lsmith77 · 2021-03-07T08:09:32Z

src/Importer/JsonResourceImporter.php

 use FriendsOfSylius\SyliusImportExportPlugin\Exception\ImporterException;
 use FriendsOfSylius\SyliusImportExportPlugin\Processor\ResourceProcessorInterface;

 final class JsonResourceImporter extends ResourceImporter implements SingleDataArrayImporterInterface
 {
    public function __construct(
-        ObjectManager $objectManager,
+        EntityManagerInterface $objectManager,


why was this change needed?

ie. why the switch to the ORM specific interface

I was encountering an error in both importers when trying to run any export/import commands, something of the sort

Fatal Error: Argument 1 passed to XXX must be an instance of Doctrine\Common\Persistence\ObjectManager, instance of ContainerNrPkzm2\EntityManager_9a5be93 given

The short version is that to circumvent this, I changed these two typehints to reflect the object that was being injected 😄

The long version: I scratched my head at this for a while as

ObjectManager is an interface

EntityManagerInterface itself extends ObjectManager
But there seemed to be shenanigans I did not understand with what is being injected. ContainerNrPkzm2\EntityManager_9a5be93 itself seems to implement the wrong thing.

Some source: https://stackoverflow.com/a/45678423/2926899

can you have a look at what class_implements returns?

https://github.com/mvpatel/symfony/blob/master/vendor/doctrine/orm/lib/Doctrine/ORM/EntityManagerInterface.php

can do 👍

You should be able to typehint Doctrine\Persistence\ObjectManager, I don't think I've ever seen the deprecation notice before where it specifically calls out to use the ORM's interface TBH. But, I did just monkey patch an app I'm trying to finish upgrading to Sylius 1.9 with the persistence namespace change and had zero issue using the base ObjectManager.

this remains the only concern I have left over merging this PR .. I have reached out to the Symfony community for advice once more.

do we have a test case that fails if we revert this change? if not, could we add one?

“ You’re using the persistence interface to autowire the entity manager. In future versions of DoctrineBundle (next major release) you’ll have to either alias this yourself or, better yet, inject the manager registry”

https://symfony-devs.slack.com/archives/C3FQPE6LE/p1616851875129800?thread_ts=1616793098.118500&channel=C3FQPE6LE&message_ts=1616851875.129800

I followed the advice you guys gave and reverted the typing and tested the import command and everything worked fine. I still think there is something there: both https://github.com/doctrine/DoctrineBundle/blob/2.2.x/UPGRADE-2.0.md#service-aliases and the fact that I did encounter the error from the original comment is strange 🤔

It would be great if someone else could give the import command a try and confirm that it does indeed work just fine with the ObjectManager typing 🙏

tl;dr it works just fine with the ObjectManager typing, as @mbabker implied 👍

lsmith77 · 2021-03-07T08:10:50Z

thank you!

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

Gogopex · 2021-03-07T10:29:44Z

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 · 2021-03-07T10:31:08Z

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 · 2021-03-07T10:54:49Z

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 · 2021-03-07T10:57:34Z

I could imagine using composer 2 could solve the memory issue

…k to ObjectManager typing in two services

…d up dependencies to ^4.4||^5.2

Composer updates

sylius-labs/coding-standard update

additional bundle updates

Gogopex · 2021-03-07T17:28:12Z

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 !

…n deprecated

composer.json

src/DependencyInjection/Configuration.php

oallain

Thanks @Gogopex for this PR :)

Travis is indeed in perpetual error, it's annoying.
To switch to Github Action, you can get inspiration from the PluginSkeleton file
https://github.com/Sylius/PluginSkeleton/blob/1.8/.github/workflows/build.yml

If you don't have time to do it, say it and I will try to do it quickly to validate your PR because she is important 🚀

Gogopex · 2021-03-15T20:26:28Z

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 · 2021-03-15T21:57:24Z

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 · 2021-03-15T22:00:03Z

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

* github action PoC from sylius invoicing plugin * Legacy build to build * dependencies updates * update build * update matrix dependency exclusion rules * forgot ^4.2 version of Symfony * legacy_build no longer relevant * removed travis.yml, updated composer.json * dropped 1.7 support * add workflow dispatch to be able to manually trigger the job * update github action * matrix order * ga fix * removed useless gulp steps * PluginSkeleton 1.9 pipeline * updated tests/Application based on SyliusSkeletonPlugin * action fix * fix * updated phpstan.neon * fix phpstan step * version bump + vendor change * version changes

Gogopex · 2021-03-24T17:31:04Z

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

I've based most of build pipeline on SyliusSkeletonPlugin's as per @oallain suggestion
I had to update the tests/Application folder. Again, I inspired myself entirely on SyliusSkeletonPlugin
There are some version changes in the composer.json, which I also took from SyliusSkeletonPlugin

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:

Someone fixes all of these PHPStan issues
We simply disable/comment/remove the PHPStan step

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 · 2021-03-24T17:48:28Z

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.

oallain

Hello @Gogopex ,

Yes, just comment the PHPStan step, we will correct it in a future PR.
You've done so much already, I think we can merge like this.

Thanks !

src/DependencyInjection/Configuration.php

* github action PoC from sylius invoicing plugin * Legacy build to build * dependencies updates * update build * update matrix dependency exclusion rules * forgot ^4.2 version of Symfony * legacy_build no longer relevant * removed travis.yml, updated composer.json * dropped 1.7 support * add workflow dispatch to be able to manually trigger the job * update github action * matrix order * ga fix * removed useless gulp steps * PluginSkeleton 1.9 pipeline * updated tests/Application based on SyliusSkeletonPlugin * action fix * fix * updated phpstan.neon * fix phpstan step * version bump + vendor change * version changes * cleaning up pipeline for merge * comment out PHPStan and PHPSpec * missing TreeBuilder arg * disable behat tests too * root to getRootNode

Gogopex · 2021-03-25T18:51:35Z

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!

* github action PoC from sylius invoicing plugin * Legacy build to build * dependencies updates * update build * update matrix dependency exclusion rules * forgot ^4.2 version of Symfony * legacy_build no longer relevant * removed travis.yml, updated composer.json * dropped 1.7 support * add workflow dispatch to be able to manually trigger the job * update github action * matrix order * ga fix * removed useless gulp steps * PluginSkeleton 1.9 pipeline * updated tests/Application based on SyliusSkeletonPlugin * action fix * fix * updated phpstan.neon * fix phpstan step * version bump + vendor change * version changes * cleaning up pipeline for merge * comment out PHPStan and PHPSpec * missing TreeBuilder arg * disable behat tests too * root to getRootNode * added back ObjectManager typing

src/Importer/ResourceImporter.php

lsmith77 · 2021-03-27T19:59:36Z

amazing work!

I have created follow up tickets for PHPStan and PHPSpec

Gogopex and others added 7 commits March 5, 2021 17:08

changed package name

008f60f

stopwatch version fix

6c0b403

updated the treebuilder in Configuration.php and replaced ObjectMappe…

b703b22

…r with EntityManagerInterface to update bundle for sylius 1.9

Update src/DependencyInjection/Configuration.php

3ca8fd7

Merge pull request #1 from MyLittleParis/sylius1.9-update

78fed76

Update bundle to make it work with Sylius 1.9

fixed returns

91232da

Merge pull request #2 from MyLittleParis/sylius1.9-update

fe051e4

fixed returns

Gogopex requested review from lsmith77 and oallain as code owners March 6, 2021 23:01

Gogopex commented Mar 6, 2021

View reviewed changes

composer.json Outdated Show resolved Hide resolved

Update composer.json

132be1a

Gogopex mentioned this pull request Mar 6, 2021

Stopwatch bump ver #250

Closed

Gogopex added 2 commits March 7, 2021 01:06

using this bundle until FoSylius merges update PR

b1341a9

updated services.yml formatting for symf4.4+

b86fc6d

lsmith77 reviewed Mar 7, 2021

View reviewed changes

travis.yml update for Sylius 1.9

0e9cce6

Gogopex and others added 9 commits March 7, 2021 12:45

updated two additional commands to Symfony 4.4+ standard, swapped bac…

12d0e72

…k to ObjectManager typing in two services

changed travis config, symfony dependency 5.2 to 4.4

b30c0fe

updated composer.json -> replaced | by || as | is deprecated -> bumpe…

9caf4ac

…d up dependencies to ^4.4||^5.2

remove PHP 7.2 pipeline for Sylius 1.9 as it only supports ^7.3

1bb891d

updated composer.json for sylius 1.9

b42dfa3

Merge pull request #3 from MyLittleParis/composer-updates

2c6b206

Composer updates

sylius-labs/coding-standard update

cd9dcc5

Merge pull request #4 from MyLittleParis/composer-updates-2

5fc1c4c

sylius-labs/coding-standard update

additional bundle updates

daf5f9a

Merge pull request #5 from MyLittleParis/composer-updates-2

4033c8c

additional bundle updates

Gogopex added 2 commits March 7, 2021 18:40

added int to return

c1f0b43

replaced ObjectManager typing by EntityManagerInterface as it has bee…

5cc5a7c

…n deprecated

lsmith77 reviewed Mar 7, 2021

View reviewed changes

composer.json Outdated Show resolved Hide resolved

lsmith77 reviewed Mar 7, 2021

View reviewed changes

composer.json Outdated Show resolved Hide resolved

lsmith77 reviewed Mar 7, 2021

View reviewed changes

src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

Gogopex commented Mar 7, 2021

View reviewed changes

src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

Update src/DependencyInjection/Configuration.php

5b70d47

oallain reviewed Mar 15, 2021

View reviewed changes

github action PoC from sylius invoicing plugin (#6)

922ebf0

Gogopex added 2 commits March 16, 2021 00:39

GitHub action (#8)

a667967

oallain reviewed Mar 24, 2021

View reviewed changes

mbabker reviewed Mar 24, 2021

View reviewed changes

src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

lsmith77 mentioned this pull request Mar 25, 2021

fixed the plugin for use in sylius 1.9 - Hotfix #252

Closed

lsmith77 reviewed Mar 27, 2021

View reviewed changes

src/Importer/ResourceImporter.php Outdated Show resolved Hide resolved

revert EntityManagerInterface typing

f40511a

lsmith77 merged commit c61b08b into FriendsOfSylius:master Mar 27, 2021

This was referenced Mar 27, 2021

fix PHPStan #255

Open

fix PHPSpec #256

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Gogopex commented Mar 6, 2021 •

edited

Loading

lsmith77 Mar 7, 2021

lsmith77 Mar 7, 2021

Gogopex Mar 7, 2021 •

edited

Loading

lsmith77 Mar 7, 2021

Gogopex Mar 7, 2021

mbabker Mar 24, 2021

lsmith77 Mar 26, 2021

lsmith77 Mar 26, 2021

lsmith77 Mar 27, 2021

Gogopex Mar 27, 2021

lsmith77 commented Mar 7, 2021

Gogopex commented Mar 7, 2021

lsmith77 commented Mar 7, 2021

Gogopex commented Mar 7, 2021

lsmith77 commented Mar 7, 2021

Gogopex commented Mar 7, 2021 •

edited

Loading

oallain left a comment

Gogopex commented Mar 15, 2021 •

edited

Loading

oallain commented Mar 15, 2021

lsmith77 commented Mar 15, 2021

Gogopex commented Mar 24, 2021

lsmith77 commented Mar 24, 2021

oallain left a comment

Gogopex commented Mar 25, 2021 •

edited

Loading

lsmith77 commented Mar 27, 2021

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

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

Conversation

Gogopex commented Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gogopex Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsmith77 commented Mar 7, 2021

Gogopex commented Mar 7, 2021

lsmith77 commented Mar 7, 2021

Gogopex commented Mar 7, 2021

lsmith77 commented Mar 7, 2021

Gogopex commented Mar 7, 2021 • edited Loading

oallain left a comment

Choose a reason for hiding this comment

Gogopex commented Mar 15, 2021 • edited Loading

oallain commented Mar 15, 2021

lsmith77 commented Mar 15, 2021

Gogopex commented Mar 24, 2021

lsmith77 commented Mar 24, 2021

oallain left a comment

Choose a reason for hiding this comment

Gogopex commented Mar 25, 2021 • edited Loading

lsmith77 commented Mar 27, 2021

Gogopex commented Mar 6, 2021 •

edited

Loading

Gogopex Mar 7, 2021 •

edited

Loading

Gogopex commented Mar 7, 2021 •

edited

Loading

Gogopex commented Mar 15, 2021 •

edited

Loading

Gogopex commented Mar 25, 2021 •

edited

Loading