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

Improve the extension #55

Open
ondrejmirtes opened this issue Apr 8, 2019 · 10 comments

Comments

2 participants
@ondrejmirtes
Copy link

commented Apr 8, 2019

Hi, I'd like to discourage you from using the CompilerExtension concept. The fact that PHPStan uses Nette\DI is not a public API and can break. Also, it means that this PHPStan extension is not usable with phpstan-shim (PHAR) where Nette is prefixed.

Also, I will remove the global variables in the next version, I didn't realize that someone would access and read them intentionally - they're not part of PHPStan's official API :)

@mglaman

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

CompilerExtension - will PHPStan provide any way to integration with the container? Otherwise the bootstrap or autoload files have no way of interacting with PHPStan -- even if the current setup is a hack.

Global variables -- I'm looking to move from those and should have used %currentWorkingDirectory%, but "it worked" when first starting :)

@ondrejmirtes

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

What exactly are you doing in the CompilerExtension? If you just need to run some code, you can create a file and put it to autoload_files in extension.neon.

@mglaman

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

Drupal doesn't have all of its classes dumped to the autoloader. We need to find the Drupal root and then do namespace processing. For this I need to know the current working directory, and autoload_files nor bootstrap have access to the container to know those params without globals.

Basically the extension, right now, uses that info and pumps data into globals for use later.

@ondrejmirtes

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

I see :) I'll try to come up with a solution.

@mglaman

This comment has been minimized.

Copy link
Owner

commented Apr 9, 2019

I'll see what I can do, as well. Now that I have PHPUnit tests it's a lot less... magical of an integration.

@ondrejmirtes

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

Hi, this should help you to move the logic into autoload_files:

phpstan/phpstan@4824bba

(You should not override bootstrap, that's for the root project that uses PHPStan.)

@mglaman mglaman added this to To do in PHPStan + Drupal May 7, 2019

@ondrejmirtes

This comment has been minimized.

Copy link
Author

commented May 10, 2019

I just released 0.11.6 with this change, so you can change your code and start requiring this version :)

@mglaman mglaman self-assigned this May 10, 2019

mglaman added a commit that referenced this issue May 10, 2019

@mglaman mglaman referenced a pull request that will close this issue May 10, 2019

Open

GH-55 Improve extension integration with PHPStan #71

@mglaman

This comment has been minimized.

Copy link
Owner

commented May 10, 2019

This helped remove some of the magical globals.

However, it cannot be fully used. Parameters need to be added to the container for service detection. Opened an initial branch and experimenting.

@ondrejmirtes

This comment has been minimized.

Copy link
Author

commented May 11, 2019

@mglaman

This comment has been minimized.

Copy link
Owner

commented May 11, 2019

:) Yeah. I'm going to get creative with it. The main thing is providing dynamic return types for services. Which requires knowing where Drupal is so extensions can be discovered. Not a hard problem. Just picking the less hacky option out of the choices

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.