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.

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

Conversation

deguif
Copy link

@deguif deguif commented Sep 6, 2022

No description provided.

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.

@@ -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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants