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

phpstorm.meta.php support for dynamic return types #2219

Draft
wants to merge 1 commit into
base: 1.10.x
Choose a base branch
from

Conversation

DCoderLT
Copy link

@DCoderLT DCoderLT commented Feb 4, 2023

This PR starts to add support for PHPStorm’s metadata files. It only supports a few basic directives for resolving method/function return types, but of course support for other directives can be added.

I do not think this is ready for a merge yet, I would like some feedback before I dig deeper into it. For example:

  • which other meta directives need to be supported (registerArgumentsSet)?
  • what tests are needed?
  • PHPStorm scans the entire project looking for these meta files, including vendor directories. Should PHPStan also look for vendored meta files by default? I’m a bit concerned about parsing time for large projects.

… it up to the lazy return type extension provider to generate extension services dynamically
@@ -219,6 +219,9 @@ parameters:
editorUrlTitle: null
errorFormat: null
__validate: true
phpStormMeta:
metaPaths:
- '.phpstorm.meta.php'
Copy link
Member

Choose a reason for hiding this comment

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

What does this path mean?

  1. How does PhpStorm search for these files? Can they always be expected to be alongside composer.json?
  2. Is the file always named the same?

I'm clinging towards only looking at the project file for now, I've never seen a public package to include this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

here's an example of one in mockery/mockery
https://github.com/mockery/mockery/blob/master/.phpstorm.meta.php

Copy link
Author

Choose a reason for hiding this comment

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

According to PHPStorm’s documentation (link), I would say it scans the entire project for files and directories with this name, and parses them all. This implementation only looks for it next to composer.json, but that is not the only place where PHPStorm would look.

I found the following packages with this file in my projects: nesbot/carbon, phpunit/phpunit, mockery/mockery, league/commonmark, nette/di, nette/utils. There are also several files with this name inside jetbrains/phpstorm-stubs, and there’s thomas-schulz/symfony-meta that provides these definitions for various Symfony components.

I think that searching the entire project (or at least vendor/) would give the closest experience to PHPStorm. But I can see arguments against this search as well, e.g. performance, metadata conflict resolution.

Copy link
Member

Choose a reason for hiding this comment

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

I think the way forward is to describe behaviour with advanced PHPDoc features which there are plenty of already, and PhpStorm starts supporting them too. That's why I think we don't need to look in vendor.

I think we should look for these files in %composerAutoloaderProjectPaths%. These are the paths where PHPStan looks for composer.json files.

Also, to be honest - I'm not sure I want to merge and maintain these 800 lines of code if the decided way forward for the community is advanced and already implemented PHPDoc features like conditional types, assert tags etc.

Maybe I can give you the needed hooks so that you can register these extensions in your own package instead?

Thank you for understanding.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I can see your reasoning about leaving this out. Adding a hook in LazyDynamicReturnTypeExtensionRegistryProvider to register these extensions would be enough, then I could leave this in a separate package. Even without such a hook, a separate package would still work, the users would just have to declare those extensions manually.

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