yireo / Yireo_ExtensionChecker

Magento 2 module to check upon the code of Magento 2 modules from the CLI
90 stars 16 forks source link

Dependency possible not needed. #16

Closed BorisovskiP closed 2 years ago

BorisovskiP commented 2 years ago

Running bin/magento yireo_extensionchecker:scan on a module throws the following errors:

Dependency "Magento_Widget" from module.xml possibly not needed. Dependency "magento/module-widget" from composer.json possibly not needed.

After double checking said module, we do indeed use Magento\Widget\Block\BlockInterface and have a dependency on it.

This also happens on other modules, for example in modules where we use Magento_Store (Magento\Store\Model\ScopeInterface) the same error is thrown:

Dependency "Magento_Store" from module.xml possibly not needed. Dependency "magento/module-store" from composer.json possibly not needed.

This happens on Magento version 2.4.3-p2, yireo/magento2-extensionchecker version 1.2.3.

jissereitsma commented 2 years ago

Good point, thanks. Note that the messages are not errors but mere suggestions. But indeed it is the opposite of what you would expect. A new version 1.2.4 now picks up on the interfaces properly. Does that fix it for you?

BorisovskiP commented 2 years ago

Not really. Dependency "magento/module-widget" from composer.json possibly not needed. is still shown. We also get a false positive where Dependency "Magento_Store" not found module.xml is thrown but that module is not used anywhere. Adding Magento_Store to module.xml will remove the notice, but it seems wrong to add a dependency when its not being used.

jissereitsma commented 2 years ago

Could you run the command together with flag -v to add verbose output and add the output here?

BorisovskiP commented 2 years ago

Checking Test_AddWidget

Dependency "Magento_Store" not found module.xml Dependency "magento/module-widget" from composer.json possibly not needed.

jissereitsma commented 2 years ago

The message Dependency "magento/module-widget" from composer.json possibly not needed could have been generated due to failure of the code to trace the PHP class back to the composer package. I've added a couple of improvements in 1.2.6 to fix this. It works on my end for this specific widget module.

As for the Magento_Store, any module that ships with Blocks requires the Store (even on the admin level) to be initialized. This is typically demonstrated with integration tests. Does your module have a Block? Then, the Magento_Store requirement is indeed needed. It's open for discussion but not a false positive.

BorisovskiP commented 2 years ago

The updated version now displays even more messages:

No PHP classes detected
Dependency "Magento_Catalog" from module.xml possibly not needed.
Dependency "Magento_Checkout" from module.xml possibly not needed.
Dependency "Magento_Widget" from module.xml possibly not needed.
Dependency "magento/module-catalog" from composer.json possibly not needed.
Dependency "magento/module-checkout" from composer.json possibly not needed.
Dependency "magento/module-widget" from composer.json possibly not needed.

The module has only one class with the following dependencies:

use Magento\Catalog\Api\ProductRepositoryInterface;
use Magento\Catalog\Model\Product;
use Magento\Checkout\Helper\Cart as CartHelper;
use Magento\Framework\App\ActionInterface;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Framework\Url\EncoderInterface;
use Magento\Framework\View\Element\Template;
use Magento\Framework\View\Element\Template\Context;
use Magento\Widget\Block\BlockInterface;
use RuntimeException;

This is how module.xml looks like :

 <module name="Magento_Catalog"/>
 <module name="Magento_Checkout"/>
 <module name="Magento_Widget"/>
 <module name="Magento_Store"/>

This is how compose.json looks:

 "magento/framework": "~103.0",
 "magento/module-catalog": "~104.0",
 "magento/module-checkout": "~100.4",
 "magento/module-widget": "~101.2",
 "php": "~7.4.0"
jissereitsma commented 2 years ago

Could you refer to the total PHP contents or the GitHub repository? Looking at what you pasted there is not much I can guess. The ExtensionChecker uses PHP Reflection underneath to scan for both source files and to convert the source back into package names. Simple typos or simply structures that I didn't think of yet, disrupt the parsing of tokens and therefore the hard guessing work. If you could dive into this yourself, I would be grateful because this is too hard to just copy and forth back, without getting to the essence.

jissereitsma commented 2 years ago

Also, which Magento version, which PHP version and which ExtensionChecker version are you using?

BorisovskiP commented 2 years ago

@jissereitsma sorry for the late reply. I create a small module which you can put under app/code and test the issue i described. You can find it here: https://github.com/BorisovskiP/extention-checker-test

This was tested on Magento 2.4.3-p2, PHP 7.4 and yireo/magento2-extensionchecker 1.2.6

jissereitsma commented 2 years ago

That's really strange. I'm trying with the same Magento, PHP and extension version, but am not getting issue with the code you mentioned. I've bumped the version to 1.2.7 just to make sure that everyone is using the same version. Could you try again?

BorisovskiP commented 2 years ago

@jissereitsma after updating i still get the same notices as before. I also asked one of my colleagues to test this independently and they got the same exact result.

jissereitsma commented 2 years ago

It almost seems as if there is a complete another version installed. I see no other solution than to compare hashes. For instance, running composer show yireo/magento2-extensionchecker gives me the following:

...
source   : [git] https://github.com/yireo/Yireo_ExtensionChecker.git 78e41d006971c68e3f7f78bc1a2b05a678df823c
...

That specific hash 78e41d006971c68e3f7f78bc1a2b05a678df823c corresponds with with the hash mentioned here for version 1.2.7: https://packagist.org/packages/yireo/magento2-extensionchecker#1.2.7

If the hash is identical, we can confirm that the source would be the same. But I've tried to run your test module against the extension checker v1.2.7 now under Magento 2.4.4 (PHP 8.1), Magento 2.4.3-p2 (PHP 7.4) and still get zero output.

Maybe to compare things further, I can also run things with the -v flag:

bin/magento yireo_extensionchecker:scan --module Test_TestModule -v

Which gives the following output:

* PHP class detected: Test\TestModule\Model\TestClass
* PHP dependency detected: Magento\Framework\View\Element\Template\Context
* PHP dependency detected: Magento\Catalog\Api\ProductRepositoryInterface
* PHP dependency detected: Magento\Checkout\Helper\Cart
* PHP dependency detected: Magento\Framework\Url\EncoderInterface
* PHP dependency detected: array
* PHP dependency detected: Magento\Framework\View\Element\BlockInterface
* PHP dependency detected: ArrayAccess
* PHP dependency detected: Magento\Widget\Block\BlockInterface
> pre-command-run: Laminas\DependencyPlugin\DependencyRewriterPluginDelegator->onPreCommandRun
> pre-command-run: Magento\ComposerRootUpdatePlugin\Plugin\PluginDefinition->checkForDeprecatedRequire
> command: MagentoHackathon\Composer\Magento\Plugin->onCommandEvent
* Reflection exception in class inspector [array]: Class "array" does not exist
* Failed to get load class: ArrayAccess

Some error, but that should be harmless for now.

BorisovskiP commented 2 years ago

The hash looks the same : 78e41d006971c68e3f7f78bc1a2b05a678df823c. Running bin/magento yireo_extensionchecker:scan --module Test_TestModule -v gives the following:


* No PHP classes detected
Dependency "Magento_Catalog" from module.xml possibly not needed.
Dependency "Magento_Checkout" from module.xml possibly not needed.
Dependency "Magento_Widget" from module.xml possibly not needed.
Dependency "magento/module-catalog" from composer.json possibly not needed.
Dependency "magento/module-checkout" from composer.json possibly not needed.
Dependency "magento/module-widget" from composer.json possibly not needed.

Maybe it will make no difference, but do you think that since we use warden that this might have some kind of an unexpected effect ?

jissereitsma commented 2 years ago

Slowly we're making progress :) Bear with me, because I think we are hunting down an interesting bug. The hash is the same, so we're dealing with the same kind of files. It slowly get the feeling that it might be related to a specific PHP version with or without specific PHP modules.

Your output shows that no classes are detected. So somehow the detection of Test\TestModule\Model\TestClass fails. This could be related to filesystem permissions (Warden?) where the PHP code can't be properly read. Or it could be due to differences in the Reflection API.

Could you first test under the new release v1.2.8? I've added various exceptions to the debugging statements, to make it hopefully a bit more clear.

BorisovskiP commented 2 years ago

Ok this looks a bit different now:

* Class is empty for file "/var/www/html/app/code/Test/TestModule/registration.php"
* PHP class detected: Test\TestModule\Model\TestClass
* PHP dependency detected: Magento\Framework\View\Element\Template\Context
* PHP dependency detected: Magento\Catalog\Api\ProductRepositoryInterface
* PHP dependency detected: Magento\Checkout\Helper\Cart
* PHP dependency detected: Magento\Framework\Url\EncoderInterface
* PHP dependency detected: Magento\Framework\View\Element\BlockInterface
* PHP dependency detected: Magento\Widget\Block\BlockInterface
jissereitsma commented 2 years ago

This seems to have solved the issue already: The starred output now shows the class is properly detected and without the verbose flag the output should be empty.

But why?

The last release was really just a large refactoring adding exceptions and more. And looking back at the commit, I can't see anything that might have caused things to have suddenly worked. The Reflection part and the file detection part didn't change.

Anyway, it seems to be working now.

BorisovskiP commented 2 years ago

Maybe one more thing that still comes up:

If you rename Model to Block, then Dependency "Magento_Store" not found module.xml will be displayed. Adding <module name="Magento_Store"/> to module.xml will fix the warning, however we also need to add "magento/module-store": "~101.1", to composer.json which will return Dependency "magento/module-store" from composer.json possibly not needed.

I updated https://github.com/BorisovskiP/extention-checker-test/tree/master/Test/TestModule to match this case.

BorisovskiP commented 2 years ago

@jissereitsma did you have a chance to look at the last comment ?

jissereitsma commented 2 years ago

Yes, it is still on my todo list.

jissereitsma commented 2 years ago

I was able to work on this, confirm the issue. There was a lot of other refactoring to be done, for instance to make sure that any missing Magento was also double-checked with any missing composer package, and vice versa. After the refactoring the issue seemed to have solved itself.

Could you verify this with the latest version?

If so, let's close this issue right away. If something new pops up, it is cleaner to create a new issue instead.

BorisovskiP commented 2 years ago

Thanks for the fix. While running the command manually, i still see the same exact warnings as from https://github.com/yireo/Yireo_ExtensionChecker/issues/16#issuecomment-1108627345. However, since we run this as a pre-push hook, and there it works, i think we can ignore it, as we will most probably never run it manually.

BorisovskiP commented 2 years ago

Sorry for the confusion, but we still see the issue from https://github.com/yireo/Yireo_ExtensionChecker/issues/16#issuecomment-1108627345. The reason why i said this works now is that we had ran bin/magento module:status --enabled | grep Test_ | while read -r MODULE; do echo "Checking $MODULE"; bin/magento yireo_extensionchecker:scan --module "$MODULE" --hide-needless 1 --hide-deprecated 1 || exit; done.

If we remove --hide-needless 1 then we get the issue from the comment again.

Adding -v to the command we get:

Warning: Use of undefined constant T_NAME_QUALIFIED - assumed 'T_NAME_QUALIFIED' (this will throw an Error in a future version of PHP) in /var/www/html/vendor/yireo/magento2-extensionchecker/Scan/ClassCollector.php on line 64
Class is empty for file "/var/www/html/src/Test/TestModule/registration.php"
No PHP classes detected
Dependency "Magento_Catalog" from module.xml possibly not needed.
Dependency "Magento_Checkout" from module.xml possibly not needed.
Dependency "Magento_Widget" from module.xml possibly not needed.
Dependency "magento/module-catalog" from composer.json possibly not needed.
Dependency "magento/module-checkout" from composer.json possibly not needed.
Dependency "magento/module-widget" from composer.json possibly not needed.
BorisovskiP commented 2 years ago

@jissereitsma did you manage to check out the issue i had from my last comment ?

jissereitsma commented 2 years ago

It could very well be that PHP 7.4 is to blame for this, or actually PHP 8.0 which introduced new namespace tokens, so that the old ones can't be used anymore. Version 1.2.14 is now released and should be able to fix this. At least in my case, this causes the check on the module Test_TestModule under PHP 8 and PHP 7.4 to be blank (because there is nothing to fix).

norgeindian commented 2 years ago

Possibly unrelated, as I'm using version 2.0.2, but also found an interesting issue. The output of the check is the following:

Module "External_Module" is possibly not needed in module.xml       
Composer requirement "external/module" possibly not needed 

But I would say, we actually need this, as we have a preference on a model in this external module. <preference for="External\Module\Model\Carrier" type="Internal\Module\Model\Carrier" /> Additionally, we actually extend this model then in our preference as well: class Carrier extends \External\Module\Model\Carrier

So in my understanding this output should definitely not happen, right? This issue came up after updating to 2.0.2, was not there before. Any idea, what I should check for further debugging?

jissereitsma commented 2 years ago

Let me try to analyse things. First of all, the DI definition (etc/di.xml) was never taken into account yet. With the new architecture, I plan to write Component Detectors (actually, the DI one is already there but empty: https://github.com/yireo/Yireo_ExtensionChecker/blob/master/ComponentDetector/DiComponentDetector.php) to pick up on similar scans: PHTML templates, DI XML files, XML layout files, JS files. So that's underway.

But, if the dependency Internal\Module\Model\Carrier is a class of the module in question and if that class extends External\Module\Model\Carrier, then the analysis of the PHP code should pick up this dependency already. Is it so that the class extending the other class is part of this very module?

norgeindian commented 2 years ago

@jissereitsma , yes, this is definitely the case. Shall I create a test module, where you can reproduce the issue?

jissereitsma commented 2 years ago

Yes, please do. I've been releasing various bug fixes since, but I'm still not able to reproduce it - as of yet.

norgeindian commented 2 years ago

@jissereitsma , just tried to test my test module, but it seems as if your newest version is again not compatible with PHP 7.4 ? At least, I get the following error message:

Parse error: syntax error, unexpected ')', expecting variable (T_VARIABLE) in /var/www/html/vendor/yireo/magento2-extensionchecker/ComponentDetector/LayoutComponentDetector.php on line 23

Would you be so kind and check that again? Then I will be happy to give you a testing module to check.

sprankhub commented 2 years ago

That was not too hard to fix... ;-)

jissereitsma commented 2 years ago

Haha. Easy peasy. I would like to keep this ticket open though, so I only close this AFTER I've made sure that this typo of error doesn't occur anymore.

norgeindian commented 2 years ago

The latest updates seem to solve the issue I had. At least, the module, which I mentioned earlier, is tested now without any issues.

BorisovskiP commented 2 years ago

@jissereitsma, here is an example that i think should be handled:

If we have a module in which we disable an event from a third party module, then we should have a dependency on this third party module. So for example if Module1 disables an even inModule2, then the extension checker should recognise that we have/need a dependency on Module2.

Another thing would be if you have something like <update handle="hyva_modal"/> or <div x-data="{...hyva.modal(), in a pthml template, then a dependency on the Hyva_Theme should be required

jissereitsma commented 2 years ago

Yes, that's definitely the idea. For this, the concept of ComponentDetectors has been introduced. Currently, there already is a component detector for the XML layout. But I didn't come up with logic here yet so that a handle like hyva_modal is picked up yet. Likewise, there is going to be a JS detector. And apparently, for Hyva, there needs to be a check for PHTML templates as well.

jissereitsma commented 2 years ago

I've added a new issue with the functionality you described: https://github.com/yireo/Yireo_ExtensionChecker/issues/31 As for this specific issue here, I think it can be closed because the fixes are now there.