Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generics annotations for translatable behavior #710

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add generics annotations for translatable behavior #710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/Contract/Entity/TranslatableInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,29 @@

use Doctrine\Common\Collections\Collection;

/**
* @template T of TranslationInterface
*/
interface TranslatableInterface
{
/**
* @return Collection<string, TranslationInterface>
* @return Collection<string, T>
*/
public function getTranslations();

/**
* @return Collection<string, TranslationInterface>
* @return Collection<string, T>
*/
public function getNewTranslations(): Collection;

/**
* @param T $translation
*/
public function addTranslation(TranslationInterface $translation): void;

/**
* @param T $translation
*/
public function removeTranslation(TranslationInterface $translation): void;

/**
Expand All @@ -28,6 +37,8 @@ public function removeTranslation(TranslationInterface $translation): void;
* newTranslations collection. In order to persist new translations, call mergeNewTranslations method, before flush
*
* @param string $locale The locale (en, ru, fr) | null If null, will try with current locale
*
* @return T
*/
public function translate(?string $locale = null, bool $fallbackToDefault = true): TranslationInterface;

Expand All @@ -44,5 +55,8 @@ public function setDefaultLocale(string $locale): void;

public function getDefaultLocale(): string;

/**
* @return class-string<T>
*/
public static function getTranslationEntityClass(): string;
}
12 changes: 12 additions & 0 deletions src/Contract/Entity/TranslationInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,24 @@

namespace Knp\DoctrineBehaviors\Contract\Entity;

/**
* @template T of TranslatableInterface
*/
interface TranslationInterface
{
/**
* @return class-string<T>
*/
public static function getTranslatableEntityClass(): string;

/**
* @param T $translatable
*/
public function setTranslatable(TranslatableInterface $translatable): void;

/**
* @return T
*/
public function getTranslatable(): TranslatableInterface;

public function setLocale(string $locale): void;
Expand Down
29 changes: 25 additions & 4 deletions src/Model/Translatable/TranslatableMethodsTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@
use Knp\DoctrineBehaviors\Contract\Entity\TranslationInterface;
use Knp\DoctrineBehaviors\Exception\TranslatableException;

/**
* @template T of TranslationInterface
*/
trait TranslatableMethodsTrait
{
/**
* @return Collection<string, TranslationInterface>
* @return Collection<string, T>
*/
public function getTranslations()
{
Expand All @@ -25,8 +28,8 @@ public function getTranslations()
}

/**
* @param Collection<string, TranslationInterface> $translations
* @phpstan-param iterable<TranslationInterface> $translations
* @param Collection<string, T> $translations
* @phpstan-param iterable<T> $translations
*/
public function setTranslations(iterable $translations): void
{
Expand All @@ -38,7 +41,7 @@ public function setTranslations(iterable $translations): void
}

/**
* @return Collection<string, TranslationInterface>
* @return Collection<string, T>
*/
public function getNewTranslations(): Collection
{
Expand All @@ -50,13 +53,19 @@ public function getNewTranslations(): Collection
return $this->newTranslations;
}

/**
* @param T $translation
*/
public function addTranslation(TranslationInterface $translation): void
{
$this->getTranslations()
->set($translation->getLocale(), $translation);
$translation->setTranslatable($this);
}

/**
* @param T $translation
*/
public function removeTranslation(TranslationInterface $translation): void
{
$this->getTranslations()
Expand All @@ -69,6 +78,8 @@ public function removeTranslation(TranslationInterface $translation): void
* newTranslations collection. In order to persist new translations, call mergeNewTranslations method, before flush
*
* @param string $locale The locale (en, ru, fr) | null If null, will try with current locale
*
* @return T
*/
public function translate(?string $locale = null, bool $fallbackToDefault = true): TranslationInterface
{
Expand Down Expand Up @@ -117,6 +128,9 @@ public function getDefaultLocale(): string
return $this->defaultLocale;
}

/**
* @return class-string<T>
*/
public static function getTranslationEntityClass(): string
{
return static::class . 'Translation';
Copy link
Author

@deguif deguif Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPStan is complaining here as the class is not ensured to exist.
I see two alternatives (other than ignoring the errors) :

  • Remove the @return class-string<T>.
  • Add a check on the class name to check it exists (and throw an exception if it doesn't exist).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one would you prefer and why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class-string<> brings value here for static analysis as it can detect when class name is invalid / not existing.

I would be in favor to keep it, but it will require to add boilerplate on the default implementation code to check that the class exists before and maybe also check that's it's an implementation of TranslationInterface. When overriding the trait method, simply returning class::name will allow static analyzer to do its job and ensure the class is valid and an instance of TranslationInterface. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest... I don't know. The reason is I don't have time to maintain this package and give the propper care it needs.

It step up few years ago, when we used it at work and needed new PHP version support. Yet haven't used it since.

I have other OS project I want to focus on.

Saying that, I'll be stepping down as maintainer and give space someone to take over.

Expand All @@ -128,6 +142,8 @@ public static function getTranslationEntityClass(): string
* newTranslations collection. In order to persist new translations, call mergeNewTranslations method, before flush
*
* @param string $locale The locale (en, ru, fr) | null If null, will try with current locale
*
* @return T
*/
protected function doTranslate(?string $locale = null, bool $fallbackToDefault = true): TranslationInterface
{
Expand Down Expand Up @@ -183,6 +199,8 @@ protected function proxyCurrentLocaleTranslation(string $method, array $argument

/**
* Finds specific translation in collection by its locale.
*
* @return T|null
*/
protected function findTranslationByLocale(string $locale, bool $withNewTranslations = true): ?TranslationInterface
{
Expand Down Expand Up @@ -228,6 +246,9 @@ private function ensureIsIterableOrCollection($translations): void
);
}

/**
* @return T|null
*/
private function resolveFallbackTranslation(string $locale): ?TranslationInterface
{
$fallbackLocale = $this->computeFallbackLocale($locale);
Expand Down
8 changes: 5 additions & 3 deletions src/Model/Translatable/TranslatablePropertiesTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,20 @@
namespace Knp\DoctrineBehaviors\Model\Translatable;

use Doctrine\Common\Collections\Collection;
use Knp\DoctrineBehaviors\Contract\Entity\TranslationInterface;

/**
* @template T of \Knp\DoctrineBehaviors\Contract\Entity\TranslationInterface
*/
trait TranslatablePropertiesTrait
{
/**
* @var Collection<string, TranslationInterface>
* @var Collection<string, T>
*/
protected $translations;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the PHPStorm autocomplete.
Could you share the gif with verification it won't happen?

Thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's a little too early as template with restriction (@template T of ..) will be available in version 2022.3

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is now working since PHPStorm introduced support for generics and this notation.


/**
* @see mergeNewTranslations
* @var Collection<string, TranslationInterface>
* @var Collection<string, T>
*/
protected $newTranslations;

Expand Down
10 changes: 10 additions & 0 deletions src/Model/Translatable/TranslatableTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,18 @@

namespace Knp\DoctrineBehaviors\Model\Translatable;

/**
* @template T of \Knp\DoctrineBehaviors\Contract\Entity\TranslationInterface
*/
trait TranslatableTrait
{
/**
* @template-use TranslatablePropertiesTrait<T>
*/
use TranslatablePropertiesTrait;

/**
* @template-use TranslatableMethodsTrait<T>
*/
use TranslatableMethodsTrait;
}
10 changes: 10 additions & 0 deletions src/Model/Translatable/TranslationMethodsTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,14 @@
use Knp\DoctrineBehaviors\Contract\Entity\TranslatableInterface;
use Nette\Utils\Strings;

/**
* @template T of TranslatableInterface
*/
trait TranslationMethodsTrait
{
/**
* @return class-string<T>
*/
public static function getTranslatableEntityClass(): string
{
// By default, the translatable class has the same name but without the "Translation" suffix
Expand All @@ -17,6 +23,8 @@ public static function getTranslatableEntityClass(): string

/**
* Sets entity, that this translation should be mapped to.
*
* @param T $translatable
*/
public function setTranslatable(TranslatableInterface $translatable): void
{
Expand All @@ -25,6 +33,8 @@ public function setTranslatable(TranslatableInterface $translatable): void

/**
* Returns entity, that this translation is mapped to.
*
* @return T
*/
public function getTranslatable(): TranslatableInterface
{
Expand Down
7 changes: 4 additions & 3 deletions src/Model/Translatable/TranslationPropertiesTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@

namespace Knp\DoctrineBehaviors\Model\Translatable;

use Knp\DoctrineBehaviors\Contract\Entity\TranslatableInterface;

/**
* @template T of \Knp\DoctrineBehaviors\Contract\Entity\TranslatableInterface
*/
trait TranslationPropertiesTrait
{
/**
Expand All @@ -16,7 +17,7 @@ trait TranslationPropertiesTrait
/**
* Will be mapped to translatable entity by TranslatableSubscriber
*
* @var TranslatableInterface
* @var T
*/
protected $translatable;
}
10 changes: 10 additions & 0 deletions src/Model/Translatable/TranslationTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,18 @@

namespace Knp\DoctrineBehaviors\Model\Translatable;

/**
* @template T of \Knp\DoctrineBehaviors\Contract\Entity\TranslatableInterface
*/
trait TranslationTrait
{
/**
* @template-use TranslationPropertiesTrait<T>
*/
use TranslationPropertiesTrait;

/**
* @template-use TranslationMethodsTrait<T>
*/
use TranslationMethodsTrait;
}