Rewrite Translatable/Translation to use Interfaces instead of Traits #387
Conversation
|
Thanks @NiR- for the follow up and the credits. Sorry I didn't have the time to do the follow up. |
|
No worries, actually we were not really active on this repository in the previous months, too. |
| @@ -43,11 +43,11 @@ public function getNewTranslations() | |||
| /** | |||
| * Adds new translation. | |||
| * | |||
| * @param Translation $translation The translation | |||
| * @param TranslationsInterface $translation The translation | |||
There was a problem hiding this comment.
Should be TranslationInterface (singular)
| 'Knp\DoctrineBehaviors\Model\Translatable\Translatable', | ||
| 'Knp\DoctrineBehaviors\Model\Translatable\Translation', | ||
| array('Knp\DoctrineBehaviors\Model\Translatable\Translatable', 'Knp\DoctrineBehaviors\Model\Translatable\TranslatableProperties'), | ||
| array('Knp\DoctrineBehaviors\Model\Translatable\Translation','Knp\DoctrineBehaviors\Model\Translatable\TranslationProperties',), |
There was a problem hiding this comment.
Maybe it could be good to also have a test in case where a non array value (eg an object) is passed here instead of an array ?
| * | ||
| * @return $this | ||
| */ | ||
| public function setTranslatable($translatable); |
There was a problem hiding this comment.
I added it but I'm not sure if I should add other missing types (notably return types) in this PR or create a dedicated one, mostly because I would have to modify traits (I wouldn't make the diff cluttered). WDYT?
There was a problem hiding this comment.
Thanks, yes I agree that return typehints should be added in a separate PR :)
| @@ -68,11 +68,11 @@ public function loadClassMetadata(LoadClassMetadataEventArgs $eventArgs) | |||
| return; | |||
| } | |||
|
|
|||
| if ($this->isTranslatable($classMetadata)) { | |||
| if ($this->requiresTranslatableMapping($classMetadata)) { | |||
There was a problem hiding this comment.
The previous isTranslatable name was more understandable IMHO, and also in a way more accurate as requires could make think of a state awareness. WDYT?
There was a problem hiding this comment.
I did the change thinking the old names were better, but then I realized that both method have a different meaning, even if they do the exact same thing. That is, an entity can implement TranslatableInterface an be a translatable, but still not require automatic mapping (that's what we try to determine by detecting if the entity uses some specific traits).
Now that said, I realize we should rather get rid of that trait detection, and just automatically add doctrine mapping as it's done right now (thus, manual mapping won't be overwritten). But the compatibility layer needed to give a smooth upgrade plan to users would be a bit more complex).
| $this->mapTranslatable($classMetadata); | ||
| } | ||
|
|
||
| if ($this->isTranslation($classMetadata)) { | ||
| if ($this->requiresTranslationMapping($classMetadata)) { |
|
Covered by #442 |
This is PR #322 rebased on current master branch and with minor fixes. Credits go to @lemoinem
Left to do: