[CreditMemo] Refactor creating CreditMemo to use a factory #295
Conversation
GSadee
force-pushed
the
credit-memo-factory
branch
from
acf9cdd to
dc54ca7
Compare
| $creditMemo = $this->creditMemoFactory->createNew(); | ||
|
|
||
| return $creditMemo; |
There was a problem hiding this comment.
| $creditMemo = $this->creditMemoFactory->createNew(); | |
| return $creditMemo; | |
| $return $this->creditMemoFactory->createNew(); |
Maybe this will work?
There was a problem hiding this comment.
I need to add a type hint to this variable because of static analysis
| @@ -0,0 +1,91 @@ | |||
| <?php | |||
|
|
|||
| declare(strict_types=1); | |||
There was a problem hiding this comment.
What about license blocks? Here and in interface? We don't use them here? (ps. and in specs)
There was a problem hiding this comment.
We don't have them in almost the entire project, but we probably should 🤔 I've added it at least in these new classes
There was a problem hiding this comment.
Should be handled by the coding standard tool.
| public function getLineItems(): Collection | ||
| { | ||
| return $this->lineItems; | ||
| } | ||
|
|
||
| public function setLineItems(Collection $lineItems): void |
There was a problem hiding this comment.
Could it be replaced by the adder method?
Anyway, we should create an empty collection in the constructor as well - currently, it'll crash while flushing the changes if setLineItems and setTaxItems are not called.
There was a problem hiding this comment.
I've added creating collections in constructor, but I'm not convince to changing these setters (setTaxItems() and setLineItems()) to adders as these are collections that shouldn't have too many operations
| @@ -0,0 +1,91 @@ | |||
| <?php | |||
|
|
|||
| declare(strict_types=1); | |||
There was a problem hiding this comment.
Should be handled by the coding standard tool.
|
Thanks, Grzegorz! 🎉 |
…g a new field (GSadee) This PR was merged into the 1.9 branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | 1.9 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | associated with changes from Sylius/RefundPlugin#295 | License | MIT Commits ------- 77e4fe4 [Documentation] Customizing CreditMemo entity by adding a new field
Fixes #278