Sylius / AdminOrderCreationPlugin

Create orders in Sylius as an Administrator
MIT License
55 stars 50 forks source link

Extracted: Repository methods to traits (fixes #140) #146

Closed igormukhingmailcom closed 4 years ago

igormukhingmailcom commented 4 years ago

Thanks, @lchrusciel Will be great to have this pr merged and tagged...

lchrusciel commented 4 years ago

But the build is red :(

lchrusciel commented 4 years ago

I'm also considering if more query specific class wouldn't be a better solution, but it is a broader topic.

igormukhingmailcom commented 4 years ago

But the build is red :(

I have feeling the reason of red builds is somewhere outside of my PR (kinda tests failed after upgrade to 1.6). This is PR of another author which have exactly same errors at travis: https://travis-ci.org/Sylius/AdminOrderCreationPlugin/jobs/645853418#L1036

igormukhingmailcom commented 4 years ago

master branch was green at Nov 22, 2019 but not now. I've created fake PR from master just to rerun checks and have same errors at master as I have in this PR (#146): https://travis-ci.org/Sylius/AdminOrderCreationPlugin/jobs/658827291#L1061

So once master will be fixed - I will rebase my branch and it will be ready to be merged, @lchrusciel

gabiudrescu commented 4 years ago

@lchrusciel any chances this get merged to Master anytime soon?

currently, if someone is using this plugin together with another plugin that requires methods in the EntityRepository it fails with a pretty obscure error like the one below:

[2020-03-23 17:49:07] messenger.ERROR: Error thrown while handling message Setono\SyliusMailchimpPlugin\Message\Command\RepushCustomers. Sending for retry #3 using 4000 ms delay. Error: "Argument 1 passed to Setono\SyliusMailchimpPlugin\Message\Handler\RepushCustomersHandler::__construct() must be an instance of Setono\SyliusMailchimpPlugin\Doctrine\ORM\CustomerRepositoryInterface, instance of Sylius\AdminOrderCreationPlugin\Doctrine\ORM\CustomerRepository given, called in /srv/sylius/var/cache/dev/ContainerE2kvtse/getSetonoSyliusMailchimp_Message_Handler_RepushCustomersService.php on line 12" {"message":"[object] (Setono\\SyliusMailchimpPlugin\\Message\\Command\\RepushCustomers: {})","class":"Setono\\SyliusMailchimpPlugin\\Message\\Command\\RepushCustomers","retryCount":3,"delay":4000,"error":"Argument 1 passed to Setono\\SyliusMailchimpPlugin\\Message\\Handler\\RepushCustomersHandler::__construct() must be an instance of Setono\\SyliusMailchimpPlugin\\Doctrine\\ORM\\CustomerRepositoryInterface, instance of Sylius\\AdminOrderCreationPlugin\\Doctrine\\ORM\\CustomerRepository given, called in /srv/sylius/var/cache/dev/ContainerE2kvtse/getSetonoSyliusMailchimp_Message_Handler_RepushCustomersService.php on line 12","exception":"[object] (TypeError(code: 0): Argument 1 passed to Setono\\SyliusMailchimpPlugin\\Message\\Handler\\RepushCustomersHandler::__construct() must be an instance of Setono\\SyliusMailchimpPlugin\\Doctrine\\ORM\\CustomerRepositoryInterface, instance of Sylius\\AdminOrderCreationPlugin\\Doctrine\\ORM\\CustomerRepository given, called in /srv/sylius/var/cache/dev/ContainerE2kvtse/getSetonoSyliusMailchimp_Message_Handler_RepushCustomersService.php on line 12 at /srv/sylius/vendor/setono/sylius-mailchimp-plugin/src/Message/Handler/RepushCustomersHandler.php:21)"} []
lchrusciel commented 4 years ago

Hey Igor and Gabi 👋 I will ask Magda, to put it to the next sprint, so where will be much higher chance, that someone will pick it up. Thanks for heads up ;)

lchrusciel commented 4 years ago

If, in any case this is still relevant to you, please rebase to the newest master and if green I will merge it.

gabiudrescu commented 4 years ago

in case this wasn't solved by some other commit, I find it very relevant, as this prevents the usage of this plugin together with other plugins, which makes the entire plugin ecosystem become irrelevant...

gabiudrescu commented 4 years ago

if no one shows some love to this PR, I'll give it a go this weekend and maybe try to fix it myself

igormukhingmailcom commented 4 years ago

@lchrusciel , its green. Could you merge and release please?

igormukhingmailcom commented 4 years ago

@lchrusciel, this one looks good?

lchrusciel commented 4 years ago

Thanks, Igor! :tada: