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

Rework extension attribute handling for standalone modules #179

Merged

Conversation

shochdoerfer
Copy link
Member

Fixes #174

In contrast to my plan outlined in #174, I took a different route as I realized that it can be problematic to depend on Magento framework classes (loading and analyzing framework classes at the same time). Additionally, it was not possible to install PHPStan and this extension in a different directory as then the Composer autoloader would not work for the project-specific classes (e.g. for the class_exists, interface_exists, and trait_exists checks).

What happens now is that all extension_attributes.xml found in etc directories are loaded recursively and evaluated regardless of whether the module is active or not. By default, the current working directory (the directory you placed your phpstan.neon file in) will be used as a starting point. If you need to change that, you can configure the magentoRoot parameter and point it to a different location.
To deal with inactive modules, we could either add a directory blacklist configuration or add some logic to evaluate the config.php settings and only take active modules into account.

@shochdoerfer
Copy link
Member Author

@mattwellss mind checking if this implementation will work for your use case? Thanks!

@mattwellss
Copy link
Contributor

@shochdoerfer sorry about the delay! I'm working odd hours around the holidays.

I was curious about how the extension behaves when installed globally (something I've never tested). Here's what I did

  • composer global install bitexpert/phpstan-magento
  • Update paths in my phpstan.neon file to point to the new globally installed extension
  • Clean phpstan's cache
  • Run globally installed phpstan from the application directory

When testing a file calling an extension attribute method is "provided by" a disabled module, the expected error occurs:

 ------ ------------------------------------------------------------------------------------------------------------------ 
  Line   ToOrderItem.php                                                                                                   
 ------ ------------------------------------------------------------------------------------------------------------------ 
  33     Call to an undefined method Magento\InventoryApi\Api\Data\SourceExtensionInterface::getIsPickupLocationActive().  
 ------ ------------------------------------------------------------------------------------------------------------------ 

While debugging the above analysis, I see PHPStan checks for a composer autoloader in the current project. That means local/vendored code is autoloadable, and that also Magento modules are able to register themselves.

Given those results, would you be willing to go into more detail about the issues here?

I took a different route as I realized that it can be problematic to depend on Magento framework classes (loading and analyzing framework classes at the same time). Additionally, it was not possible to install PHPStan and this extension in a different directory as then the Composer autoloader would not work for the project-specific classes (e.g. for the class_exists, interface_exists, and trait_exists checks).

Specifically, I'm wondering about:

  • The issue with depending on Magento Framework classes
  • Autoloading (which I believe works fine)

I know this is bitExpert's module, and any input on its direction is at your discretion. So while I would love to maintain the ability to limit extension_attributes.xml to registered/enabled modules, it's your call in the end.


I've did check out this PR, though! It works, but doesn't identify the problem of using a nonexistent method like 0.13.0 does.

@shochdoerfer
Copy link
Member Author

@mattwellss thanks for your feedback.

So far I did not test global Composer installs, good to know that this will work fine as well. I tested installing phpstan and the extension in a different directory and then let it examine a Magento module in another directory, that was a request by someone else because they install QA tools independently from their projects. In that case Composer autoloading does not work, because Composer does only know about the local dependencies but not the ones from the other project.

None of the other phpstan extensions I checked, made use of the framework or library classes they are written for. My assumption is, that it could lead to side-effects when loading those classes and analyzing their code in parallel. In our case, having the mock/stub implementations in place (to fix doc block issues) that get loaded automatically might even cause more trouble.

For sure, I would like to support enabled/disabled modules. For now, I am just not sure if the effort would be worth it. In our own projects, we don't really have disabled modules in our code-bases. Does this happen often in your case?

Use Composer autoloader to check if the interface definition
for the extension attributes does in fact exist.
@shochdoerfer shochdoerfer added this to the 0.14.0 milestone Jan 8, 2022
@shochdoerfer shochdoerfer merged commit 4359545 into bitExpert:master Jan 8, 2022
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.

Improve extension attribute handling for standalone modules
2 participants