-
Notifications
You must be signed in to change notification settings - Fork 27
Add Interceptor Autoloader #345
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
base: master
Are you sure you want to change the base?
Add Interceptor Autoloader #345
Conversation
…Magento framework versions
- Fix non-variadic parameters being variadic - Ensure it extends the original class
- If intercepting an interface, set implemented interfaces - otherwise set extended class - Add Interceptor trait
@shochdoerfer Looking forward to your thoughts here. Unfortunately, Magento's stuff was built heavily around saving directly to a file (with a hard requirement on |
I feel your pain. This is why I built a completely separate logic to generate the code in the autoloaders. Far away from ideal but it felt way too hard to get the Magento internals to work. Plus, ideally, the PHPStan extension does not depend on any framework code as this could cause issues during the code analysis. Being quite overloaded with work these days, I think I can check this beginning of August earliest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for letting this hang a lot longer than I wanted. The last months have been too busy to tackle this.
Could you take care of my feedback? That'll be awesome.
} | ||
], | ||
"require": { | ||
"php": "^7.2.0 || ^8.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you change this because of your project requirements?
While I think the extension is technically compatible with PHP 7.x, I'd rather not like to soften it here as our CI pipeline currently only checks for PHP 8.x as it turned out way too complex to support PHP 7.x & 8.x plus all the various Magento versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you perhaps misread this change, as it removed PHP 7.2 support from the library (due to some PHP 8 only code in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I need better glasses. My bad!
*/ | ||
public function autoloaderGeneratesCacheFileWhenNotFoundInCache(): void | ||
{ | ||
// little hack: the proxy autoloader will use Reflection to look for a class without the \Proxy prefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for removing this? Is this no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, some of the work I did necessitated creating the file this was trying to avoid creating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pefect!
} | ||
} | ||
|
||
$generator = new ClassGenerator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not ideal that we rely here on code from Magento itself, as this can confuse PHPStan during the code analysis. PHPStan extension should ideally never load and execute code from the code base they analyse. That's the 2nd reason I had to implement a custom logic for the code generation.
return '<?php'.PHP_EOL.PHP_EOL.$this->_fixCodeStyle($code); | ||
} | ||
|
||
protected function _fixCodeStyle(string $sourceCode): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not in Magento, we don't need underscores to identify non-public methods :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was lifted directly out of Magento, but I'm happy to adjust it if desired :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with the function names, just the underscores have to go to make the CI pipeline happy :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take care of that next week!
This PR adds an autoloader for
...\Interceptor
classes.TODO:
Will resolve #342