Yireo_ExtensionChecker icon indicating copy to clipboard operation
Yireo_ExtensionChecker copied to clipboard

Dependency possible not needed.

Open BorisovskiP opened this issue 4 years ago • 24 comments

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.

BorisovskiP avatar Apr 19 '22 10:04 BorisovskiP

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?

jissereitsma avatar Apr 25 '22 08:04 jissereitsma

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.

BorisovskiP avatar Apr 25 '22 10:04 BorisovskiP

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

jissereitsma avatar Apr 25 '22 10:04 jissereitsma

Checking Test_AddWidget

  • PHP class detected: Test\AddWidget\Block\Widget\AddWidget
  • 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

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

BorisovskiP avatar Apr 25 '22 10:04 BorisovskiP

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.

jissereitsma avatar Apr 25 '22 12:04 jissereitsma

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"

BorisovskiP avatar Apr 25 '22 14:04 BorisovskiP

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 avatar Apr 25 '22 14:04 jissereitsma

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

jissereitsma avatar Apr 25 '22 14:04 jissereitsma

@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

BorisovskiP avatar May 18 '22 13:05 BorisovskiP

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?

jissereitsma avatar May 28 '22 08:05 jissereitsma

@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.

BorisovskiP avatar Jun 02 '22 11:06 BorisovskiP

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.

jissereitsma avatar Jun 02 '22 13:06 jissereitsma

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 ?

BorisovskiP avatar Jun 02 '22 14:06 BorisovskiP

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.

jissereitsma avatar Jun 02 '22 18:06 jissereitsma

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

BorisovskiP avatar Jun 02 '22 18:06 BorisovskiP

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.

jissereitsma avatar Jun 03 '22 05:06 jissereitsma

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 avatar Jun 03 '22 06:06 BorisovskiP

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

BorisovskiP avatar Jun 13 '22 10:06 BorisovskiP

Yes, it is still on my todo list.

jissereitsma avatar Jun 13 '22 10:06 jissereitsma

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.

jissereitsma avatar Jul 07 '22 17:07 jissereitsma

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 avatar Jul 08 '22 06:07 BorisovskiP

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 avatar Jul 08 '22 07:07 BorisovskiP

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

BorisovskiP avatar Aug 08 '22 11:08 BorisovskiP

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).

jissereitsma avatar Aug 08 '22 13:08 jissereitsma

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?

norgeindian avatar Sep 29 '22 08:09 norgeindian

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?

jissereitsma avatar Sep 29 '22 12:09 jissereitsma

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

norgeindian avatar Sep 30 '22 06:09 norgeindian

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

jissereitsma avatar Oct 03 '22 08:10 jissereitsma

@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.

norgeindian avatar Oct 05 '22 08:10 norgeindian

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

sprankhub avatar Oct 05 '22 09:10 sprankhub