Price calculators by umpirsky · Pull Request #1197 · Sylius/Sylius

umpirsky · 2014-03-13T14:15:12Z

Fixes issue #1120.

This will allow us to have custom price calculators. For example, we have something like this in our project.

<?php

namespace Sylius\Bundle\CoreBundle\Calculator;

use Sylius\Bundle\CoreBundle\Entity\Variant;
use Sylius\Bundle\CoreBundle\Provider\RegionalPriceProvider;

class RegionalPriceCalculator implements PriceCalculatorInterface
{
    private $regionalPriceProvider;

    public function __construct(RegionalPriceProvider $regionalPriceProvider)
    {
        $this->regionalPriceProvider = $regionalPriceProvider;
    }

    public function calculate(Variant $variant)
    {
        return $this->regionalPriceProvider->getRegionalCurrentPrice($variant);
    }
}

winzou · 2014-03-13T14:17:00Z

src/Sylius/Bundle/AddressingBundle/Form/EventListener/ResizeZoneMemberCollectionListener.php

Old one was nice as well ;)

It was better :)

winzou · 2014-03-13T14:18:20Z

But you display wrong price in frontend?
This calculator is nice, we shouldn't have all these calculate methods in Order and OrderItem.

umpirsky · 2014-03-13T14:19:15Z

@winzou Ah, true. Missed that :)

Will fix, thanks.

kayue · 2014-03-13T14:21:14Z

Cool stuff 👍

stloyd · 2014-03-13T14:29:51Z

src/Sylius/Bundle/CoreBundle/Calculator/DefaultPriceCalculator.php

Default calculator simply returns the variant price.

umpirsky · 2014-03-13T14:54:24Z

@winzou umpirsky@85c67f8

winzou · 2014-03-13T15:05:05Z

I would totally separate the formatting from the calculation. Like this: {{ variant.price|calculate|sylius_price }} or even in the function style (maybe more intuitive than filter): {{ calculate(variant)|sylius_price }}

kayue · 2014-03-13T15:20:23Z

Agree with @winzou on function style. Maybe calculate_price()?

And beside variant, should we accept Product?

stloyd · 2014-03-13T15:36:53Z

TBH. it's kinda confusing for me where to put this code. IMO it's not totally correct to have it in the CoreBundle, while all prices are being calculated by MoneyBundle... I'm confused.

Anyway, we can't depend on Variable only, we should support Product too, as in Product you have i.e. getMasterVariant()...

kayue · 2014-03-13T15:45:26Z

@stloyd Where does MoneyBundle calculate price? I thought MoneyBundle was just doing currency exchange.

If we put PriceCalculator to MoneyBundle, that would make MoneyBundle depends on Variant wouldn't it?

pjedrzejewski · 2014-03-13T19:22:08Z

PricingBundle. Would be cool to put it there... I'll put your implementation on steroids later. (add optional configuration to pricing calculators) What do you think?

pjedrzejewski · 2014-03-13T19:25:23Z

And I'd go with PriceableInterface. Instead of relying on the variant. It could return the price calculator name and configuration. (in future)

stloyd · 2014-03-13T19:33:47Z

@pjedrzejewski I would say that bundle is overkill, component sound ok but bundle is overkill ;)

But totally agree about that interface, I was thinking about same thing, but placed in ProductBundle, or in MoneyBundle, but prefer the first one

pjedrzejewski · 2014-03-13T20:17:07Z

IMHO, it definitely should be a separate bundle. In @umpirsky implementation it is quite thin, but for sure it will contain much more and can be used separately from Product bundle. Why would you couple the pricing stuff only to Product bundle?

umpirsky · 2014-03-13T21:00:01Z

@winzou I agree about {{ calculate(variant)|sylius_price }}, will update.

@pjedrzejewski I agree about PriceableInterface, will update. What about keeping it in core for now as long as it is this tiny. We can move it to component or separate bundle once you decide to put it on fire with configuration etc?

pjedrzejewski · 2014-03-13T21:56:11Z

@umpirsky Deal! 👍

umpirsky · 2014-03-14T13:30:35Z

@pjedrzejewski Ready for merging. Just waiting for Travis.

umpirsky · 2014-03-16T16:01:49Z

@pjedrzejewski Can you merge it please?

Price calculators

pjedrzejewski · 2014-03-16T19:07:27Z

Thanks Sasha! The build was failing previously. Nice work! 👍

umpirsky added 2 commits March 13, 2014 13:38

Make my name human readable

7ee8738

Price calculators

6f7724e

stloyd added the Enhancement label Mar 13, 2014

winzou reviewed Mar 13, 2014
View reviewed changes

stloyd reviewed Mar 13, 2014
View reviewed changes

Show calculated price in frontend

85c67f8

Introduced PriceCalculatorInterface

9fa211f

Separate price calculation and formatting

ddfc60d

pjedrzejewski pushed a commit that referenced this pull request Mar 16, 2014

Merge pull request #1197 from umpirsky/fix/issue-1120

9ff345d

Price calculators

pjedrzejewski merged commit 9ff345d into Sylius:master Mar 16, 2014

Uh oh!

Conversation

umpirsky commented Mar 13, 2014

Uh oh!

winzou Mar 13, 2014

Choose a reason for hiding this comment

Uh oh!

umpirsky Mar 13, 2014

Choose a reason for hiding this comment

Uh oh!

winzou commented Mar 13, 2014

Uh oh!

umpirsky commented Mar 13, 2014

Uh oh!

kayue commented Mar 13, 2014

Uh oh!

stloyd Mar 13, 2014

Choose a reason for hiding this comment

Uh oh!

umpirsky commented Mar 13, 2014

Uh oh!

winzou commented Mar 13, 2014

Uh oh!

kayue commented Mar 13, 2014

Uh oh!

stloyd commented Mar 13, 2014

Uh oh!

kayue commented Mar 13, 2014

Uh oh!

pjedrzejewski commented Mar 13, 2014

Uh oh!

pjedrzejewski commented Mar 13, 2014

Uh oh!

stloyd commented Mar 13, 2014

Uh oh!

pjedrzejewski commented Mar 13, 2014

Uh oh!

umpirsky commented Mar 13, 2014

Uh oh!

pjedrzejewski commented Mar 13, 2014

Uh oh!

umpirsky commented Mar 14, 2014

Uh oh!

umpirsky commented Mar 16, 2014

Uh oh!

pjedrzejewski commented Mar 16, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants