Sylius / ShopApiPlugin

Shop API for Sylius.
https://sylius.com
129 stars 89 forks source link

Allow customizing picked up cart's locale #644

Closed dnna closed 4 years ago

dnna commented 4 years ago

Currently when picking up a new cart, the default channel locale is always used.

This PR fixes this by enabling the use of an optional ?locale URL parameter to customize the cart's locale.

dnna commented 4 years ago

@lchrusciel I'm happy to make the change, but note there are multiple instances where $request->query->get('locale') is already used in the shop API.

Indicatively check the following:

dnna commented 4 years ago

I have made the requested changes so that LocaleContext is used instead of request query. To make this work, I had to create a new ChannelAndLocaleBasedRequestInterface and a ChannelAndLocaleBasedCommandProvider.

For ChannelAndLocaleBasedRequestInterface I extended the original ChannelBasedRequestInterface instead of having a completely separate interface in order to minimize BC breaks.

mamazu commented 4 years ago

Yeah localization is indeed still a problem in the ShopApi. But this looks good. I would prefer to move it in to the main provider and then add an interface that only specifies the setLocale function. And then in the provider if the interface is implemented call the function.

class PickUpCartCommand implements LocaleNeededInterface {
    private $localeCode;

    // Normal code
    public function setLocale(string $localeCode): void
    {
        $this->localeCode = $localeCode; 
    }
}

and then

public function getCommand(Request $httpRequest): CommandInterface
{
    $command = $this->transformHttpRequest($httpRequest)->getCommand();
    if ($command instanceof LocaleNeededInterface) {
        // Get locale somehow
        $command->setLocale($locale);
    }
    return $command;
}

Maybe even the LocaleAwareInterface from the Symfony translator. What do you think?

dnna commented 4 years ago

That approach makes a lot of sense. I'll try to implement it that way and get back.

dnna commented 4 years ago

@mamazu I have reimplemented it the way you mentioned. I didn't use Symfony's LocaleAwareInterface because I was afraid it might be somehow integrated with Symfony and get unexpectedly triggered by the container or request listeners, so I created a new LocaleAwareCommandInterface.

I implemented it in a way that allows localeContext to be nullable to avoid BC breaks if someone has decorated the provider or extended the command. I can't see a way that it might break but if you do please let me know.

mamazu commented 4 years ago

Looks good. This could be then extended to all other commands.

mamazu commented 4 years ago

Thank you, Dimosthenis! :tada: