[Core][Shipping] Fix shipping method choice type #5216
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pjedrzejewski
added
Enhancement
| $methods = $resolver->getSupportedMethods($options['subject']); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's move this logic to the "main" methods resolver and inject it here. It will support every shipment and use exactly this logic to get the right methods. (CompositeMethodsResolver as name, wdyt?)
There was a problem hiding this comment.
Sure 👍 Less logic in form type is always a good choice 😄
1 task
Zales0123
force-pushed
the
list-checkout-shipping-methods-also-by-channel
branch
from
445de00
to
7135621
Compare
Zales0123
force-pushed
the
list-checkout-shipping-methods-also-by-channel
branch
from
7135621
to
64c3676
Compare
| if ($this->sharedStorage->has('channel')) { | ||
| $channel = $this->sharedStorage->get('channel'); | ||
| $channel->addShippingMethod($shippingMethod); | ||
| } |
There was a problem hiding this comment.
This if should be before $this->shippingMethodRepository->add($shippingMethod);. Right now the relation is not flushed in this step.
Zales0123
force-pushed
the
list-checkout-shipping-methods-also-by-channel
branch
from
63c3f07
to
d0ee908
Compare
|
Nice work Mateusz, thanks! Note: in separate PR, we could remove the CompositeMethodsResolverInterface - This could be an implementation of ShippingMethodsResolverInterface - no need for new contract. Anyway, this is a minor improvement. 👍 |
pamil
pushed a commit
to pamil/Sylius
that referenced
this pull request
May 7, 2019
…methods-also-by-channel [Core][Shipping] Fix shipping method choice type
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem with
ShippingMethodChoiceType, as it didn't use channel to list shipping methods and also not available for channel methods have been... available.Instead of quick fix, I've rebuild deeply shipping methods resolver logic:
ShippingMethodChoiceTypeuses prioritized registry to get proper shipping methods resolverThis changes allowed to remove
ShippingMethodChoiceTypeinstance from Core. Also it's now highly simple to use own shipping methods resolver (it has to be just implemented and tagged with proper priority).