EmicoEcommerce / Magento2Tweakwise-archived

Magento 2 module for Tweakwise integration
Other
9 stars 25 forks source link

Dependencies are not documented #134

Open jissereitsma opened 4 years ago

jissereitsma commented 4 years ago

While the Magento extension is heavily relying upon numerous Magento modules, that could be enabled or disabled, and composer dependencies (while composer makes upgrades easier), almost none of these dependencies are documented in the composer.json and module.xml files. A simple output of a scanning tool I built, shows the following:

Function "json_encode" requires PHP extension "ext-json"
Function "preg_match" requires PHP extension "ext-pcre"
Dependency "Magento_Catalog" not found module.xml
Dependency "Emico_TweakwiseExport" not found module.xml
Dependency "Magento_Eav" not found module.xml
Dependency "Magento_Swatches" not found module.xml
Dependency "Magento_Tax" not found module.xml
Dependency "Magento_UrlRewrite" not found module.xml
Dependency "Magento_Store" not found module.xml
Dependency "Magento_Customer" not found module.xml
Dependency "Magento_Backend" not found module.xml
Dependency "magento/framework" not found composer.json. Current version is 103.0.0
Dependency "magento/module-catalog" not found composer.json. Current version is 104.0.0
Dependency "magento/module-eav" not found composer.json. Current version is 102.1.0
Dependency "magento/module-swatches" not found composer.json. Current version is 100.4.0
Dependency "magento/module-tax" not found composer.json. Current version is 100.4.0
Dependency "psr/log" not found composer.json. Current version is 1.1.3
Dependency "magento/module-url-rewrite" not found composer.json. Current version is 102.0.0
Dependency "magento/module-store" not found composer.json. Current version is 101.1.0
Dependency "magento/module-customer" not found composer.json. Current version is 103.0.0
Dependency "magento/module-search" not found composer.json. Current version is 101.1.0

Is this something that could be fixed?

edwinljacobs commented 4 years ago

Hi,

Yeah thats true, ill document the most exotic dependencies. In all honesty, I don't see the benefit of documenting the magento/framework dependency for example, its kind of implicit to the package type "magento2-module". What is your view on this?

WIth kind regards

jissereitsma commented 4 years ago

I think you are missing the point that composer is used for dependency management. In magento/framework, there have been numerous breaking changes that could impact third party extensions. If for instance a feature was removed in Magento 2.4, while your module was relying upon it, without declaring the dependency, customers will be able to upgrade to Magento 2.4 and find out that things are breaking in production.

I am definitely NOT saying that you should add the magento/framework with a version *. Instead, look on the Magento devdocs for all information regarding best practices.

I have heard that people complained about your module, because of basic things like this missing and 1 instance involved your module not documenting its dependencies, hence succeeding in a composer upgrade but failing in production. It is really important to realize that offering a third party extension to 1000s of developers in the Magento ecosystem requires you to be as pro-active as possible to code everything, instead of being reluctant about this. In short, read the docs of composer for best practices.

jissereitsma commented 4 years ago

A little example: The commit https://github.com/EmicoEcommerce/Magento2Tweakwise/commit/fbb81115a75469a0e2048817b2a21b998d731d86 shows that you tried to implement a new interface that is only possible with Magento 2.4. And it is NOT introduced in the Magento 2.4 package, which is just there for marketing. Instead, it is introduced in magento/framework version 103.0.0.

Introducing new features in new versions is not a bad thing. However, this would break with people still running Magento 2.3 or older. In short, the composer dependencies are there to determine which version of your module are matching which version of the framework.

I found out about this within 1 minute of reviewing your code. So please understand that composer dependencies are so common for some developers that it is easy to spot that this extension is missing them.

edwinljacobs commented 4 years ago

Fair enough, Ill add/update them, it is indeed better to force the correct constraints. To be honest the dependencies have not been on my mind the last few features. So please understand that composer dependencies are so common for some developers that it is easy to spot that this extension is missing them. I am well aware of that. Your point about the missing dependencies is valid.