set default value for Channel taxCalculationStrategy #11418

oallain · 2020-05-03T19:48:03Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	fixes #11414
License	MIT

pamil

What issue does it solve?

pamil · 2020-05-06T08:13:48Z

Oh, I've just seen #11414. I'm not feeling comfortable with hardcoding the strategy, we should find some way to at least rely on some constraint or just use the first strategy that is available.

lchrusciel · 2020-05-06T11:03:56Z

Neither do I, the current solution is a no-go right now.

Maybe we should provide such config through our factory service? This is the default method of creating new objects in Sylius.

oallain · 2020-05-15T12:42:11Z

Hi @lchrusciel @pamil ,

I create a ChannelFactory in Core, tell me if this is what you expected?

lchrusciel

Seems good to me. Thanks Olivier!

src/Sylius/Component/Core/Factory/ChannelFactory.php

lchrusciel · 2020-06-02T12:08:27Z

src/Sylius/Component/Core/Factory/ChannelFactoryInterface.php

+use Sylius\Component\Core\Model\ChannelInterface;
+use Sylius\Component\Resource\Factory\FactoryInterface;
+
+interface ChannelFactoryInterface extends FactoryInterface


Is this interface even needed, as we have the interface from component namespace?

Sylius/src/Sylius/Component/Channel/Factory/ChannelFactoryInterface.php

Line 21 in 7901f9a

public function createNamed(string $name): ChannelInterface;

If so, it should be extended rather than replaced.

IMHO, we need it while we don't have the covariance provided by PHP 7.4.

However, all of the current implementations, that are based on the previous interface cannot be changed, due to BC Policy. If we want to extend the previous implementation we need to extend the previous interface

src/Sylius/Component/Core/Factory/ChannelFactory.php

lchrusciel · 2020-06-02T12:11:06Z

Sorry for pushing it back and forth, but not spotting the current factory implementation was my mistake from the early beginning

oallain · 2020-06-15T20:18:40Z

Hi @lchrusciel

All looks good, but Travis failed.
Can you (or anyone) can help me ?

Thanks

mamazu · 2020-06-15T21:39:35Z

The reason some of your tests may fail is because currently the master is broken. So just wait for them to fix it and then rebase again. (Or fix it yourself and make a separate pull request)

lchrusciel · 2020-06-16T10:47:58Z

We can try to rebase now, it should work ;)

oallain · 2020-06-18T07:25:17Z

Hi @lchrusciel ,

Rebase is done, and "application" is green on Travis, but "doc" still red 😞

mamazu · 2020-06-18T09:57:59Z

Can you rebase again now?

oallain · 2020-06-18T11:46:52Z

@mamazu ,

It's worse 😉

mamazu · 2020-06-18T12:15:04Z

Hm, but the master is green. Very odd.

oallain · 2020-06-19T11:46:31Z

@mamazu

I changed target to master and it's green now :)

Thanks !

oallain · 2020-06-27T16:13:29Z

Hello @lchrusciel @mamazu

I rebase (again) and now Travis is green 😄 (But Symfony/Insight is Pending 😂 )

Regards

mamazu · 2020-06-29T08:39:06Z

Hm, someone from Sylius needs to restart this job.

lchrusciel · 2020-07-01T08:48:14Z

Hey Olivier 👋

I didn't find time to take over this PR, however, I won't merge it without change to #11418 (comment). Right now this PR contains BC Break.

oallain · 2020-07-07T19:44:43Z

Hello,

I am facing a dilemma and I don't know what/how to do :

Either the new Sylius\Component\Core\Factory\ChannelFactoryInterface extends Sylius\Component\Channel\Factory\ChannelFactoryInterface and in this case, the return types should be different but this is only possible with the covariance provided by PHP 7.4 - so BC with PHP < 7.4
Either the new Sylius\Component\Core\Factory\ChannelFactoryInterface return Sylius\Component\Channel\Model\ChannelInterface and not Sylius\Component\Core\Model\ChannelInterface.
Either keep this PR like this, however, all of the current implementations, that are based on the previous interface cannot be changed, like say Lukasz in set default value for Channel taxCalculationStrategy #11418 (comment) - so BC with older implementations

What is the best solution ?

oallain · 2020-07-07T20:03:35Z

I just pushed with solution 2) 😉

Looking forward to your feedback.

mamazu · 2020-07-07T20:24:13Z

I would suggest option 1 with the return type not being type-hinted but just annotated in the doc type. Then psalm or phpstan or any other static code analysis tool can pick it up and it doesn't break things. (And this is also the best way to go in my opinion)

Option 2 is not great. Because the interface of the component is smaller than the interface of the core bundle (as far as I know) so this would also be sub-optimal.

Option 3: I don't know if there is already a branch in Sylius that is for 2.0 where those BC breaks will probably be bundled into. I also made a breaking change as a PR which also still has no place to go really.

lchrusciel · 2020-07-10T09:01:06Z

Option 3 is a no-go for us. Option 2 is what we are doing for the last few years and this is what I would recommend. However, as @mamazu stated, probably improved option 1 (with docblock) is the best solution right now.

oallain · 2020-09-01T12:00:44Z

Hello, @mamazu

This rebase is 💚 (green) 🎉

lchrusciel · 2020-09-07T09:37:11Z

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "tax-calculation-strategy" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

lchrusciel · 2020-09-07T12:15:09Z

Thanks, @oallain! 🥇

…nor improvements (lchrusciel) This PR was merged into the 1.7 branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | 1.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | fixes for #11418 | License | MIT  Commits ------- 4fe2f3c [Channel] Fix bc-breaking changes in ChannelFactory

oallain requested a review from a team as a code owner May 3, 2020 19:48

oallain force-pushed the tax-calculation-strategy branch from ccaa975 to f35af58 Compare May 3, 2020 19:57

lchrusciel added DX Issues and PRs aimed at improving Developer eXperience. RFC Discussions about potential changes or new features. labels May 4, 2020

pamil reviewed May 6, 2020

View reviewed changes

lchrusciel reviewed May 22, 2020

View reviewed changes

mamazu reviewed May 23, 2020

View reviewed changes

src/Sylius/Component/Core/Factory/ChannelFactory.php Outdated Show resolved Hide resolved

oallain force-pushed the tax-calculation-strategy branch from 5cbb841 to c33cb40 Compare May 30, 2020 20:33

lchrusciel reviewed Jun 2, 2020

View reviewed changes

oallain force-pushed the tax-calculation-strategy branch 2 times, most recently from eeb1267 to 5cb43bc Compare June 3, 2020 18:59

oallain force-pushed the tax-calculation-strategy branch from 5cb43bc to da5510c Compare June 15, 2020 19:29

probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Jun 15, 2020

oallain changed the base branch from master to 1.7 June 15, 2020 19:31

oallain force-pushed the tax-calculation-strategy branch 2 times, most recently from 67c09ca to 70a40c6 Compare June 16, 2020 17:58

probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jun 16, 2020

oallain force-pushed the tax-calculation-strategy branch from 70a40c6 to efcd749 Compare June 16, 2020 17:59

oallain force-pushed the tax-calculation-strategy branch from efcd749 to 2786699 Compare June 18, 2020 10:48

oallain force-pushed the tax-calculation-strategy branch from a641300 to ff7c64b Compare June 19, 2020 11:26

oallain force-pushed the tax-calculation-strategy branch 2 times, most recently from 1734369 to 281269e Compare June 27, 2020 15:12

oallain force-pushed the tax-calculation-strategy branch from 30620d6 to 31dc666 Compare July 7, 2020 20:47

lchrusciel approved these changes Jul 10, 2020

View reviewed changes

oallain force-pushed the tax-calculation-strategy branch from 31dc666 to ef3f7fc Compare July 27, 2020 20:53

oallain force-pushed the tax-calculation-strategy branch 2 times, most recently from 94b23ef to 31f3e91 Compare September 1, 2020 08:33

lchrusciel changed the base branch from master to 1.8 September 7, 2020 09:35

lchrusciel changed the base branch from 1.8 to master September 7, 2020 09:35

oallain added 2 commits September 7, 2020 11:37

set default value for Channel taxCalculationStrategy

f4f0ed8

factory extends component factory

0dd21de

lchrusciel changed the base branch from master to 1.7 September 7, 2020 09:37

lchrusciel force-pushed the tax-calculation-strategy branch from 31f3e91 to 0dd21de Compare September 7, 2020 09:37

lchrusciel approved these changes Sep 7, 2020

View reviewed changes

lchrusciel merged commit 2436b84 into Sylius:1.7 Sep 7, 2020

lchrusciel mentioned this pull request Sep 7, 2020

[Channel] Fix bc-breaking changes in ChannelFactory and minor improvements #11814

Merged

oallain deleted the tax-calculation-strategy branch September 9, 2020 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set default value for Channel taxCalculationStrategy #11418

set default value for Channel taxCalculationStrategy #11418

oallain commented May 3, 2020 •

edited

Loading

pamil left a comment

pamil commented May 6, 2020

lchrusciel commented May 6, 2020

oallain commented May 15, 2020

lchrusciel left a comment

lchrusciel Jun 2, 2020

oallain Jun 3, 2020

lchrusciel Jun 17, 2020

lchrusciel commented Jun 2, 2020

oallain commented Jun 15, 2020

mamazu commented Jun 15, 2020

lchrusciel commented Jun 16, 2020

oallain commented Jun 18, 2020

mamazu commented Jun 18, 2020

oallain commented Jun 18, 2020

mamazu commented Jun 18, 2020

oallain commented Jun 19, 2020 •

edited

Loading

oallain commented Jun 27, 2020

mamazu commented Jun 29, 2020

lchrusciel commented Jul 1, 2020

oallain commented Jul 7, 2020

oallain commented Jul 7, 2020 •

edited

Loading

mamazu commented Jul 7, 2020 •

edited

Loading

lchrusciel commented Jul 10, 2020

oallain commented Sep 1, 2020

lchrusciel commented Sep 7, 2020

lchrusciel commented Sep 7, 2020

set default value for Channel taxCalculationStrategy #11418

set default value for Channel taxCalculationStrategy #11418

Conversation

oallain commented May 3, 2020 • edited Loading

pamil left a comment

Choose a reason for hiding this comment

pamil commented May 6, 2020

lchrusciel commented May 6, 2020

oallain commented May 15, 2020

lchrusciel left a comment

Choose a reason for hiding this comment

lchrusciel Jun 2, 2020

Choose a reason for hiding this comment

oallain Jun 3, 2020

Choose a reason for hiding this comment

lchrusciel Jun 17, 2020

Choose a reason for hiding this comment

lchrusciel commented Jun 2, 2020

oallain commented Jun 15, 2020

mamazu commented Jun 15, 2020

lchrusciel commented Jun 16, 2020

oallain commented Jun 18, 2020

mamazu commented Jun 18, 2020

oallain commented Jun 18, 2020

mamazu commented Jun 18, 2020

oallain commented Jun 19, 2020 • edited Loading

oallain commented Jun 27, 2020

mamazu commented Jun 29, 2020

lchrusciel commented Jul 1, 2020

oallain commented Jul 7, 2020

oallain commented Jul 7, 2020 • edited Loading

mamazu commented Jul 7, 2020 • edited Loading

lchrusciel commented Jul 10, 2020

oallain commented Sep 1, 2020

lchrusciel commented Sep 7, 2020

lchrusciel commented Sep 7, 2020

oallain commented May 3, 2020 •

edited

Loading

oallain commented Jun 19, 2020 •

edited

Loading

oallain commented Jul 7, 2020 •

edited

Loading

mamazu commented Jul 7, 2020 •

edited

Loading