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

Support for extension attributes #162

Conversation

mattwellss
Copy link
Contributor

@mattwellss mattwellss commented Nov 29, 2021

Resolves #112

@shochdoerfer
Copy link
Member

Thanks a lot for your hard work on this. Very much appreciated! It'll take some time until I can check your PR, I am quite busy these days.

One thing we should do is to add laminas/laminas-code as a composer dependency if possible. Technically, the PHPStan extension is supporting anything magento/framework >101.0.0 (Magento 2.2.x IIRC). Back then dependency was called zendframework/zend-code which would break your implementation. We would need to check with which version constraints laminas/laminas-code is included in the different Magento versions and then come up with needed Composer constraints.

Is this PR blocked by #161 or could we treat that independently?

@shochdoerfer shochdoerfer added this to the 0.12.0 milestone Nov 29, 2021
@shochdoerfer
Copy link
Member

@sprankhub can you help out testing the changes, please? Since you seem to have some projects that make use of extension attributes, it would be lovely to get some real world feedback. I'd need to build a quick module myself to test things.

@mattwellss
Copy link
Contributor Author

One thing we should do is to add laminas/laminas-code as a composer dependency if possible. Technically, the PHPStan extension is supporting anything magento/framework >101.0.0 (Magento 2.2.x IIRC). Back then dependency was called zendframework/zend-code which would break your implementation.

Good point. I'll check it out.

Is this PR blocked by #161 or could we treat that independently?

Not blocked, I only noticed it because in my own Magento environment I've configured PHPStan's cache to be project-local.

@shochdoerfer shochdoerfer self-assigned this Nov 29, 2021
@sprankhub
Copy link
Contributor

Well, actually it was not me, but my colleague :D I'll see what we can do regarding testing :)

@shochdoerfer
Copy link
Member

@mattwellss will take a deeper dive in this next week. Need to finish a few projects at work before my holiday next week. Should have a bit more time then. I try to get the next release including this PR done before Christmas in the best case.

@shochdoerfer
Copy link
Member

@mattwellss quick question: If I would add the phpstan-extension to a Magento module project, where would I have to point the magentoRoot parameter? In the case of this repo https://github.com/bitExpert/magento2-force-login structure what would be the correct configuration?

@mattwellss
Copy link
Contributor Author

Great question, @shochdoerfer. I'll take a look tomorrow... I can see better now your concerns about the extension being able to find extension attributes for "non-application" usage.


$autoloader->autoload($interfaceName);
static::assertTrue(interface_exists($interfaceName));
$interfaceReflection = new \ReflectionClass($interfaceName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure how to resolve this error (outside of suppressing it). PHPStan definitely cannot find this class using static analysis!

Copy link
Member

Choose a reason for hiding this comment

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

I am fine if we ignore the error. I mean me have to make some magic here, that's fine then :)

@shochdoerfer
Copy link
Member

@mattwellss did a quick check and things look good. Works even for stand-alone modules, it seems. Tested it against magento/module-inventory-api. With the ExtensionInterfaceAutoloader not being active, I get quite a few extension interface errors. After enabling the autoloader the errors are gone.

The only thing that we need to change is the laminas-code version as older Magento versions require version 3.3 (Magento 2.3.6) or 3.4.1 (Magento 2.3.7). I'll check how "compatible" the different versions are and fix them before merging your changes.

@mattwellss
Copy link
Contributor Author

did a quick check and things look good. Works even for stand-alone modules, it seems.

That's great! I've been a bit inundated with time-sensitive tasks, so I haven't been able to complete a test.

I was planning to test the efficacy of both core extension attributes and custom ones, which I imagine might fail. If you were able to verify that both are fine then I can skip it.

I'll correct the version requirement for laminas code this week.

@shochdoerfer
Copy link
Member

I was planning to test the efficacy of both core extension attributes and custom ones, which I imagine might fail. If you were able to verify that both are fine then I can skip it.

Would be cool if you could check that. I haven't done much so far with extension attributes, I don't want to miss something important :)

I'll correct the version requirement for laminas code this week.

Not needed, I am currently running some tests and have fixed it locally. Will commit it once I manually merge the PR.

@shochdoerfer
Copy link
Member

I was hoping that it would be possible to install zendframework/zend-code and laminas/laminas-code at the same time. Sadly, that is not possible. That means we need up raise the minimum requirement for magento/framework to 102.0.0 (Magento 2.3.0). Fine for me, not sure if anyone is still using this extension with a Magento 2.2 instance. And if so, they can still stick to an older version of the extension.

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.

Feature Request: Make the extension aware of Magento's extension attributes
3 participants