Separated order items subtotal calculation logic from twig extension #11191

4c0n · 2020-03-06T12:11:35Z

Q	A
Branch?	1.6
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
License	MIT

I'd like to be able to use the logic for calculating the subtotal of all order items outside of Twig templates, hence this separation.

4c0n · 2020-03-06T15:13:12Z

Sorry I don't know how to fix the memory issues on the php 7.2 installation build. The others pass.

lchrusciel · 2020-03-09T08:21:38Z

src/Sylius/Bundle/ShopBundle/Twig/OrderItemsSubtotalExtension.php

+    /** @var OrderItemsSubtotalCalculatorInterface */
+    private $calculator;
+
+    public function __construct(OrderItemsSubtotalCalculatorInterface $calculator)


It's BC break :( We should keep current logic as a fallback

@lchrusciel Thanks for the review! I addressed the BC break, please have another look.

…1191)

4c0n · 2020-04-01T07:48:34Z

@lchrusciel Is there anymore that needs to be done before this can be merged?
I'm waiting on this to release a plugin that allows setting a minimum order value. It can be done by duplicating this logic in the plugin as well, but I'd rather not.

lchrusciel

Hey Youri,

thanks a lot for your contribution. I would like to merge it in the nearest days. Could you take my comments into account?

However, we are slightly blocked by red build on v1.6 :(

src/Sylius/Bundle/ShopBundle/Tests/Calculator/OrderItemsSubtotalCalculatorTest.php

lchrusciel · 2020-04-07T10:54:11Z

src/Sylius/Bundle/ShopBundle/Tests/Calculator/OrderItemsSubtotalCalculatorTest.php

+use Sylius\Component\Core\Model\OrderInterface;
+use Sylius\Component\Core\Model\OrderItemInterface;
+
+final class OrderItemsSubtotalCalculatorTest extends TestCase


The structure of this test seems to be a perfect match for phpspec. It is not necessary but rather recommended. Would you like to give it a try?

Perhaps another time. I'm interested to know when do you prefer spec over unit when it comes to testing? Very similar results can be achieved between the two, but I guess the focus does not 100% have to be on structure alone for unit tests?

PHPSpec - unit testing, but to be more precise it is more for designing implementation. Testing every class in total separation, all collaborators are mocked and you need to be precise, what should be asserted (shouldBeCalled) and return (willReturn). Prophecy is a great part of this library, which was recently deprecated on PHPUnit. Also, it forces you for the usage of mocks rather than separated classes.
PHPUnit - functional testing - mostly used for more than one class initialized in the test suite. Some examples, testing of services fetched from the container, FormType testing, testing of configuration and old API testing
Behat - acceptance tests, focused mostly on business value and covered through all of Sylius layers

The distinction between PHPSpec & PHPUnit in terms of unit testing can indeed be a matter of taste, but we've decided to go with PHPSpec, and keeping codebase coherent is important for us.

Is it clearer right now?

Thanks, that's pretty clear. I'd hate to jump in and learn about how phpspec works right here and now, but next time I'll do it that way for sure.

src/Sylius/Bundle/ShopBundle/Twig/OrderItemsSubtotalExtension.php

Co-Authored-By: Łukasz Chruściel <lchrusciel@gmail.com>

…' of github.com:4c0n/Sylius into feature/separate-order-items-subtotal-calculation-logic

4c0n · 2020-04-07T11:22:49Z

Hey Youri,

thanks a lot for your contribution. I would like to merge it in the nearest days. Could you take my comments into account?

However, we are slightly blocked by red build on v1.6 :(

Do you have any idea about the memory issues of the 7.2 installation build? That is what's failing, maybe because of the new package not being in the composer cache or something like that.
Let's see if the new build has the same issue or not.

4c0n · 2020-04-07T12:09:22Z

Seems to be something specific to Travis:

=> Storing composer.lock in cache (cache key: e82419b5c6d676d78173acda5ffa8b24)
The command "etc/travis/run-suite install "${SYLIUS_SUITE}"" failed and exited with 1 during .
Your build has been stopped.

Is it possible for you to clear the composer cache somehow?

lchrusciel · 2020-04-07T12:12:43Z

I've restarted this build again

4c0n · 2020-04-07T12:47:11Z

Installing: CoreBundle
> composer install --ansi --no-interaction --prefer-dist --quiet
Warning: proc_open(): fork failed - Cannot allocate memory in phar:///home/travis/.phpenv/versions/7.2.29/bin/composer/vendor/symfony/console/Application.php on line 952
The following exception is caused by a lack of memory or swap, or not having swap configured
Check https://getcomposer.org/doc/articles/troubleshooting.md#proc-open-fork-failed-errors for details
PHP Warning:  proc_open(): fork failed - Cannot allocate memory in phar:///home/travis/.phpenv/versions/7.2.29/bin/composer/vendor/symfony/console/Application.php on line 952
                                                     
  [ErrorException]                                   
  proc_open(): fork failed - Cannot allocate memory

4c0n · 2020-04-07T12:53:07Z

So composer crashes because it seems to need more memory. Typical 😝

I don't usually work with Travis, so I kind of don't know if it is possible at all to increase the available memory for composer.

4c0n · 2020-04-09T08:57:36Z

@lchrusciel Hi Lukasz,
In order to resolve the memory issues, I added the creation and activation of a swap file in the CI VM.
The build passes now. Are we good to go?

lchrusciel · 2020-04-09T15:07:37Z

Hey @4c0n, In the meantime, I have fixed it in #11341, where I've reduced the number of tags to solve. We should do it anyway. Your proposal is good to go, however, I would like to revert the last change.

Nonetheless, it can be done in separate PR

lchrusciel · 2020-04-09T15:10:21Z

Thanks, Youri! 🎉

Separated order items subtotal calculation logic from twig extension

dc9abca

4c0n requested a review from a team as a code owner March 6, 2020 12:11

4c0n added 2 commits March 6, 2020 14:17

Added mockery plugin for psalm

06397e9

Enabling psalm plugin

09d39b2

Zales0123 added DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). labels Mar 9, 2020

lchrusciel reviewed Mar 9, 2020

View reviewed changes

4c0n added 3 commits March 9, 2020 09:49

Made injection of calculator in twig extension optional

40c277e

Changed todo into deprecation comment

d7f483d

Also triggering user deprecation "error"

6367184

4c0n pushed a commit to nedac-sorbo/SyliusMinimumOrderValuePlugin that referenced this pull request Mar 9, 2020

WH-124 Using Sylius fork until PR is merged/resolved (Sylius/Sylius#1…

b2b3a48

…1191)

lchrusciel approved these changes Mar 9, 2020

View reviewed changes

lchrusciel reviewed Apr 7, 2020

View reviewed changes

4c0n and others added 4 commits April 7, 2020 13:08

Update src/Sylius/Bundle/ShopBundle/Twig/OrderItemsSubtotalExtension.php

2f08497

Co-Authored-By: Łukasz Chruściel <lchrusciel@gmail.com>

Removed doc from constructor

1ad6204

Refactored unit test for the order items subtotal calculator test

f4839fb

Merge branch 'feature/separate-order-items-subtotal-calculation-logic…

7cca8bc

…' of github.com:4c0n/Sylius into feature/separate-order-items-subtotal-calculation-logic

Attempting to create swap space to accomodate composer's memory issues

0f64d06

lchrusciel merged commit ade9a9e into Sylius:1.6 Apr 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separated order items subtotal calculation logic from twig extension #11191

Separated order items subtotal calculation logic from twig extension #11191

4c0n commented Mar 6, 2020

4c0n commented Mar 6, 2020

lchrusciel Mar 9, 2020

4c0n Mar 9, 2020

4c0n commented Apr 1, 2020

lchrusciel left a comment

lchrusciel Apr 7, 2020

4c0n Apr 7, 2020

lchrusciel Apr 7, 2020

4c0n Apr 7, 2020

4c0n commented Apr 7, 2020

4c0n commented Apr 7, 2020

lchrusciel commented Apr 7, 2020

4c0n commented Apr 7, 2020

4c0n commented Apr 7, 2020

4c0n commented Apr 9, 2020

lchrusciel commented Apr 9, 2020

lchrusciel commented Apr 9, 2020

Separated order items subtotal calculation logic from twig extension #11191

Separated order items subtotal calculation logic from twig extension #11191

Conversation

4c0n commented Mar 6, 2020

4c0n commented Mar 6, 2020

lchrusciel Mar 9, 2020

Choose a reason for hiding this comment

4c0n Mar 9, 2020

Choose a reason for hiding this comment

4c0n commented Apr 1, 2020

lchrusciel left a comment

Choose a reason for hiding this comment

lchrusciel Apr 7, 2020

Choose a reason for hiding this comment

4c0n Apr 7, 2020

Choose a reason for hiding this comment

lchrusciel Apr 7, 2020

Choose a reason for hiding this comment

4c0n Apr 7, 2020

Choose a reason for hiding this comment

4c0n commented Apr 7, 2020

4c0n commented Apr 7, 2020

lchrusciel commented Apr 7, 2020

4c0n commented Apr 7, 2020

4c0n commented Apr 7, 2020

4c0n commented Apr 9, 2020

lchrusciel commented Apr 9, 2020

lchrusciel commented Apr 9, 2020