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

inline annotations for the interfaces - what is the correct way? #1369

Closed
vitek-rostislav opened this issue Sep 4, 2019 · 4 comments
Closed
Labels
DX & Refactoring Requests for DX improvements and refactorings Help wanted We need your help or opinion how to resolve this Stale This issue have not been touched for some time and if it doesn't change, it will be closed soon.

Comments

@vitek-rostislav
Copy link
Contributor

What is happening

In tests, we often access services using DI container directly - e.g.

/** @var \Shopsys\FrameworkBundle\Component\Domain\Domain $domain */
$domain = $this->container->get(Domain::class);

However, when grabbing an interface this way, we are not consistent with the annotations, you can see that often there is no @var annotation at all, or we annotate it with a particular framework implementation, like that:

/** @var \Shopsys\FrameworkBundle\Model\Product\ProductDataFactory $productDataFactory */
$productDataFactory = $this->getContainer()->get(ProductDataFactoryInterface::class);

Is there any reason why not to annotate it with

\Shopsys\FrameworkBundle\Model\Product\ProductDataFactoryInterface

directly?

The inconsistency might cause problems to annotations fixer implemented in #1344 as it has no information that it should replace

\Shopsys\FrameworkBundle\Model\Product\ProductDataFactory

in the annotation with

\Shopsys\ShopBundle\Model\Product\ProductDataFactory

Actually, the fixer looks for

\Shopsys\FrameworkBundle\Model\Product\ProductDataFactoryInterface

as this is the "parent" for project class that is registered in DIC.

Expected result

We know how to use inline annotations for interfaces and it is implemented or documented.

@vitek-rostislav vitek-rostislav added the DX & Refactoring Requests for DX improvements and refactorings label Sep 4, 2019
@LukasHeinz LukasHeinz added the Help wanted We need your help or opinion how to resolve this label Sep 5, 2019
@jDolba
Copy link
Contributor

jDolba commented Sep 9, 2019

what about not using this annotations at all?

most of the examples in tests (with direct access to container) might/should be covered by .phpstorm.meta.php file
example:
https://gist.github.com/bcremer/be2a558a452d4f45dc04994ffe7b8292
or
http://php-di.org/doc/ide-integration.html#metadata-file

general definition in phpstorm.meta.php file might be
override(\Psr\Container\ContainerInterface::get(0), type(0));

$domain = $this->container->get(\Domain::class);
// PHP STORM IDE should know now the $domain is "\Domain"
  • maybe it will be possible to generate phpstorm.meta file from compiled container? ( similarly to phing annotations-fix logic?)

for static analysis (phpstan) there are already extensions (https://github.com/phpstan/phpstan-symfony/blob/master/README.md) so php-stan should be able to analyze without php-docs

so basically the questions is:
are the annotations intended for IDE? if so: is phpstorm recommended IDE? if so -> use phpstorm.meta file

@vitek-rostislav
Copy link
Contributor Author

Hi @jDolba, thanks for your insight, it sounds good to me 👍

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because there was no activity within the last 4 months (and it is quite a long time). It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale This issue have not been touched for some time and if it doesn't change, it will be closed soon. label Jan 10, 2020
@stale
Copy link

stale bot commented Mar 11, 2020

This issue has been automatically closed because there was no acivity within the last half a year.

@stale stale bot closed this as completed Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX & Refactoring Requests for DX improvements and refactorings Help wanted We need your help or opinion how to resolve this Stale This issue have not been touched for some time and if it doesn't change, it will be closed soon.
Projects
None yet
Development

No branches or pull requests

3 participants