[WIP] Default shipping and payment method resolver #11482

vvasiloi · 2020-05-17T14:05:41Z

Default shipping method resolver and default payment method resolver should use the corresponding collection resolver to get available methods to choose from.

Right now the logic is duplicated and in case a custom collection resolver uses a different logic it might happen that the default resolver will return a method which is not available.

Need suggestions for tests and on how to deprecate the current logic, especially the constructor arguments. Also, should I split it 2 PRs?

TODO:

Tests
Deprecation of current logic, including constructor arguments
UPGRADE-*.md

lchrusciel

Hey Victor!

thanks for your contribution.

I would say, we are fine with specs only. Another possibility is to write more complicated functional tests, but it may be tricky, as we will need to wire it up manually. /cc @Sylius/core-team

src/Sylius/Component/Core/Resolver/EligibleDefaultShippingMethodResolver.php

vvasiloi · 2020-06-10T11:24:47Z

@lchrusciel any updates on this?

lchrusciel · 2020-06-17T10:41:06Z

Let's go with this PR as it is, without splitting. Looking right now at the code, specs are a must. Here we need to cover at least changes to the logic. Also, we could easily test that some argument is not needed. From a high-level perspective, we could add one scenario for each case, where the resolver will return different objects than the current solution. If you have an idea for PHPUnit(which I mentioned previously) go for it, but it would just skip this part.

This is a nice addition, and I would like to merge it. Is there anything else I can help you with to push it forward?

vvasiloi · 2020-06-17T11:52:01Z

@lchrusciel Thank you! There's one thing that I'm not sure how to do and I need help.
How do I deprecate passing of the 1st argument (the repository) and also deprecate not passing the second one (the resolver) without a BC break?
Considering that the 1st parameter's type and name must remain unchanged so it won't break service arguments auto-wiring.

lchrusciel · 2020-06-18T09:52:24Z

I missed this case, and to be sure I wasn't aware about issue you've raised. However, I cannot see too many possibilities other than just message, that only the second argument can be passed and it will become the first one.

I'm considering also, that maybe some bind option could provide backward compatibility in case you raised. Then, we can remove typehint from the constructor, inject new service, bind this argument for autowiring and then in code just handle different behavior depending on injected service.

stale · 2020-09-20T04:26:08Z

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

vvasiloi · 2020-09-20T06:06:33Z

Don't stale.

…thod in "sylius.shipping_method_resolver.default"

…od in "sylius.payment_method_resolver.default"

add spec for the check

…ution to Sylius#12891)

vvasiloi · 2021-08-14T14:29:58Z

@lchrusciel can't believe that it's been more than a year since I opened this.
Time to get back to work.
Could you please take a look at it again?
I added a deprecation notice when passing the repository, a check for neither the repository nor the resolver is passed and the specs.

src/Sylius/Component/Core/Resolver/DefaultPaymentMethodResolver.php

stloyd · 2021-08-14T15:39:42Z

src/Sylius/Component/Core/Resolver/DefaultPaymentMethodResolver.php


-        $paymentMethods = $this->paymentMethodRepository->findEnabledForChannel($channel);
        if (empty($paymentMethods)) {


Suggested change

if (empty($paymentMethods)) {

if (!$paymentMethods) {

Maybe count($paymentMethods) === 0?
Or use reset($paymentMethods) and check if it's false instead of relying on the numeric index ($paymentMethods[0]).

stloyd · 2021-08-14T15:40:15Z

src/Sylius/Component/Core/Resolver/EligibleDefaultShippingMethodResolver.php

+    {
+        $shippingMethods = $this->shippingMethodsResolver->getSupportedMethods($shipment);
+
+        if (empty($shippingMethods)) {


Suggested change

if (empty($shippingMethods)) {

if (!$shippingMethods) {

Same as https://github.com/Sylius/Sylius/pull/11482/files#r688957860

…12891)

…hen passing it

add spec for the check

jakubtobiasz · 2023-02-07T14:15:41Z

Hi @vvasiloi!
Any plans to continue work on it? PR is old, and we'd like to close it if there are no further plans to finish it :D.

vvasiloi · 2023-02-07T14:36:03Z

The question is what does it take for it to be finished?

jakubtobiasz · 2023-02-07T15:14:53Z

@vvasiloi I dunno, you're the author 😂. I see there's a WIP in the title that's why I'm asking :D. Also there're conflicts.

vvasiloi · 2023-02-07T15:57:49Z

Well, now I have to resolve the conflict, so yeah, it's WIP.

vvasiloi requested a review from a team as a code owner May 17, 2020 14:05

lchrusciel reviewed May 22, 2020

View reviewed changes

src/Sylius/Component/Core/Resolver/EligibleDefaultShippingMethodResolver.php Outdated Show resolved Hide resolved

src/Sylius/Component/Core/Resolver/EligibleDefaultShippingMethodResolver.php Outdated Show resolved Hide resolved

stale bot added the Stale Issues and PRs with no recent activity, about to be closed soon. label Sep 20, 2020

stale bot removed the Stale Issues and PRs with no recent activity, about to be closed soon. label Sep 20, 2020

lchrusciel added the Do not stale Important issues and PRs, that should not be stalled by Stale Bot label Sep 21, 2020

vvasiloi added 2 commits August 14, 2021 15:24

use "sylius.shipping_methods_resolver" to get the default shipping me…

2b6448c

…thod in "sylius.shipping_method_resolver.default"

use "sylius.payment_methods_resolver" to get the default payment meth…

b5e1a7d

…od in "sylius.payment_method_resolver.default"

vvasiloi force-pushed the default_resolvers branch from 5fcaff3 to b5e1a7d Compare August 14, 2021 13:24

vvasiloi added 7 commits August 14, 2021 15:28

remove 'deprecated' tags from private methods

53b42be

add spec

ce2233e

update deprecation version

06d4cb2

use the methods resolver by default

0753f9a

make repository optional and add a deprecation notice when passing it

5be0e09

add a check that at least the repository or the resolver is passed

18d4127

add spec for the check

replace properties type annotations with built-in type hints (contrib…

91f5e3a

…ution to Sylius#12891)

stloyd reviewed Aug 14, 2021

View reviewed changes

vvasiloi added 4 commits August 14, 2021 18:19

replace properties type annotations with built-in type hints (Sylius#…

60aa36e

…12891)

add spec for DefaultPaymentMethodResolver with $paymentMethodsResolver

2ba6417

make $paymentMethodRepository optional and add a deprecation notice w…

a373948

…hen passing it

add a check that at least the repository or the resolver is passed

ad5692f

add spec for the check

jakubtobiasz self-assigned this Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Default shipping and payment method resolver #11482

[WIP] Default shipping and payment method resolver #11482

vvasiloi commented May 17, 2020 •

edited

lchrusciel left a comment

vvasiloi commented Jun 10, 2020

lchrusciel commented Jun 17, 2020

vvasiloi commented Jun 17, 2020 •

edited

lchrusciel commented Jun 18, 2020

stale bot commented Sep 20, 2020

vvasiloi commented Sep 20, 2020

vvasiloi commented Aug 14, 2021

stloyd Aug 14, 2021

vvasiloi Aug 14, 2021

stloyd Aug 14, 2021

vvasiloi Aug 14, 2021

jakubtobiasz commented Feb 7, 2023

vvasiloi commented Feb 7, 2023

jakubtobiasz commented Feb 7, 2023 •

edited

vvasiloi commented Feb 7, 2023


		$paymentMethods = $this->paymentMethodRepository->findEnabledForChannel($channel);
		if (empty($paymentMethods)) {

[WIP] Default shipping and payment method resolver #11482

Are you sure you want to change the base?

[WIP] Default shipping and payment method resolver #11482

Conversation

vvasiloi commented May 17, 2020 • edited

lchrusciel left a comment

Choose a reason for hiding this comment

vvasiloi commented Jun 10, 2020

lchrusciel commented Jun 17, 2020

vvasiloi commented Jun 17, 2020 • edited

lchrusciel commented Jun 18, 2020

stale bot commented Sep 20, 2020

vvasiloi commented Sep 20, 2020

vvasiloi commented Aug 14, 2021

stloyd Aug 14, 2021

Choose a reason for hiding this comment

vvasiloi Aug 14, 2021

Choose a reason for hiding this comment

stloyd Aug 14, 2021

Choose a reason for hiding this comment

vvasiloi Aug 14, 2021

Choose a reason for hiding this comment

jakubtobiasz commented Feb 7, 2023

vvasiloi commented Feb 7, 2023

jakubtobiasz commented Feb 7, 2023 • edited

vvasiloi commented Feb 7, 2023

vvasiloi commented May 17, 2020 •

edited

vvasiloi commented Jun 17, 2020 •

edited

jakubtobiasz commented Feb 7, 2023 •

edited