[Translation] Remove fluent interfaces #3315
Conversation
CoderMaggie
commented
Sep 21, 2015
CoderMaggie
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | yes |
| Deprecations? | no |
| Fixed tickets | no |
| License | MIT |
CoderMaggie
changed the title
| /** | ||
| * @author Gonzalo Vilaseca <gvilaseca@reiss.co.uk> | ||
| */ | ||
| class AbstractTranslation implements TranslationInterface | ||
| { | ||
| /** | ||
| * Locale. | ||
| * | ||
|
|
There was a problem hiding this comment.
This shouldn't be added 😉
|
https://github.com/TheMadeleine/Sylius/blob/translation-fluent-interfaces/src/Sylius/Component/Translation/Model/AbstractTranslatable.php#L82 - this method should be on the interface |
| @@ -44,10 +40,6 @@ public function getTranslatable() | |||
| */ | |||
| public function setTranslatable(TranslatableInterface $translatable = null) | |||
| { | |||
| if ($translatable === $this->translatable) { | |||
michalmarcinkowski
added
the
Enhancement
| @@ -44,10 +40,6 @@ public function getTranslatable() | |||
| */ | |||
| public function setTranslatable(TranslatableInterface $translatable = null) | |||
| { | |||
| if ($translatable === $this->translatable) { | |||
| return $this; | |||
There was a problem hiding this comment.
As above should be reverted, this line should be just return;.
CoderMaggie
force-pushed
the
translation-fluent-interfaces
branch
from
866f86a
to
4bbbe08
Compare
|
@michalmarcinkowski 👍 done. |
| @@ -19,15 +19,11 @@ | |||
| abstract class AbstractTranslatable implements TranslatableInterface | |||
| { | |||
| /** | |||
| * Translations. | |||
| * | |||
| * @var TranslationInterface[] | |||
There was a problem hiding this comment.
Collection|TranslationInterface[]
There was a problem hiding this comment.
IMHO putting Collection here is a bad idea as we don't want to couple this interface with Doctrine Commons.
There was a problem hiding this comment.
But it's already coupuled with them in __construct()...
There was a problem hiding this comment.
Ok, constructor in abstract class is not the thing I would expect :) Anyway, that should apply to TranslatableInterface.
There was a problem hiding this comment.
@pamil decoupling from Doctrine Common is a topic for a separate discussion and is not concern of this PR.
There was a problem hiding this comment.
@michalmarcinkowski Sure, that's why I want to prevent coupling at the moment :)
There was a problem hiding this comment.
Let's stick to current convention to avoid inconsistency and if we decide to decouple we will do it everywhere at once.
|
FWIW I'm working on a PR that gets rid of the abstract translation classes in favour of traits. |
|
@gonzalovilaseca that is great! If I will merge this PR will it make your rebase harder? |
|
@michalmarcinkowski Indeed ... but I might just copy this code to the trait, so go ahead, merge if you want and I'll find my way :-) |
[Translation] Remove fluent interfaces
|
Thank you Magda! 👍 |
