magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.51k stars 9.31k forks source link

module.xml is parsed even though extension is not enabled via config.php #887

Closed fooman closed 9 years ago

fooman commented 9 years ago

It appears that the module.xml gets parsed even though the extension has not been explicitly enabled. For example this app/code/Namespace/Module/etc/module.xml file (note the incorrect use of version vs schema_version):

<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../../../lib/internal/Magento/Framework/Module/etc/module.xsd">
    <module name="Namespace_Module" version="2.0.0"/>
</config>

will take down the store with this error message:

a:4:{i:0;s:81:"Attribute 'schema_version' is missing for module 'Namespace_Module'.";i:1;s:4087:"#0 /lib/internal/Magento/Framework/Module/ModuleList/Loader.php(56): Magento\Framework\Module\Declaration\Converter\Dom->convert(Object(DOMDocument))
#1 /lib/internal/Magento/Framework/Module/ModuleList.php(70): Magento\Framework\Module\ModuleList\Loader->load()
#2 /lib/internal/Magento/Framework/Module/ModuleList.php(90): Magento\Framework\Module\ModuleList->getAll()
#3 /lib/internal/Magento/Framework/Module/DbVersionInfo.php(153): Magento\Framework\Module\ModuleList->getOne('Magento_Core')
#4 /lib/internal/Magento/Framework/Module/DbVersionInfo.php(64): Magento\Framework\Module\DbVersionInfo->isModuleVersionEqual('Magento_Core', '2.0.0')
#5 /lib/internal/Magento/Framework/Module/DbVersionInfo.php(89): Magento\Framework\Module\DbVersionInfo->isSchemaUpToDate('Magento_Core', 'core_setup')
#6 /lib/internal/Magento/Framework/Module/Plugin/DbStatusValidator.php(51): Magento\Framework\Module\DbVersionInfo->getDbVersionErrors()
#7 [internal function]: Magento\Framework\Module\Plugin\DbStatusValidator->aroundDispatch(Object(Magento\Framework\App\FrontController\Interceptor), Object(Closure), Object(Magento\Framework\App\Request\Http))
#8 /lib/internal/Magento/Framework/Interception/Chain/Chain.php(62): call_user_func_array(Array, Array)
#9 /lib/internal/Magento/Framework/Interception/Chain/Chain.php(57): Magento\Framework\Interception\Chain\Chain->invokeNext('Magento\Framewo...', 'dispatch', Object(Magento\Framework\App\FrontController\Interceptor), Array, 'front-controlle...')
#10 /app/code/Magento/PageCache/Model/App/FrontController/VarnishPlugin.php(53): Magento\Framework\Interception\Chain\Chain->Magento\Framework\Interception\Chain\{closure}(Object(Magento\Framework\App\Request\Http))
#11 [internal function]: Magento\PageCache\Model\App\FrontController\VarnishPlugin->aroundDispatch(Object(Magento\Framework\App\FrontController\Interceptor), Object(Closure), Object(Magento\Framework\App\Request\Http))
#12 /lib/internal/Magento/Framework/Interception/Chain/Chain.php(62): call_user_func_array(Array, Array)
#13 /var/generation/Magento/Framework/App/FrontController/Interceptor.php(90): Magento\Framework\Interception\Chain\Chain->invokeNext('Magento\Framewo...', 'dispatch', Object(Magento\Framework\App\FrontController\Interceptor), Array, 'front-controlle...')
#14 /app/code/Magento/PageCache/Model/App/FrontController/BuiltinPlugin.php(66): Magento\Framework\App\FrontController\Interceptor->Magento\Framework\App\FrontController\{closure}(Object(Magento\Framework\App\Request\Http))
#15 [internal function]: Magento\PageCache\Model\App\FrontController\BuiltinPlugin->aroundDispatch(Object(Magento\Framework\App\FrontController\Interceptor), Object(Closure), Object(Magento\Framework\App\Request\Http))
#16 /var/generation/Magento/Framework/App/FrontController/Interceptor.php(95): call_user_func_array(Array, Array)
#17 /var/generation/Magento/Framework/App/FrontController/Interceptor.php(119): Magento\Framework\App\FrontController\Interceptor->___callPlugins('dispatch', Array, Array)
#18 /lib/internal/Magento/Framework/App/Http.php(114): Magento\Framework\App\FrontController\Interceptor->dispatch(Object(Magento\Framework\App\Request\Http))
#19 /lib/internal/Magento/Framework/App/Bootstrap.php(244): Magento\Framework\App\Http->launch()
#20 /pub/index.php(36): Magento\Framework\App\Bootstrap->run(Object(Magento\Framework\App\Http))
#21 {main}";s:3:"url";s:34:"/pub/errors/default/css/styles.css";s:11:"script_name";s:10:"/index.php";}

I believe this contradicts the stated goal of the config.php approach to separate code being present and code being executed after a deployment.

TexanHogman commented 9 years ago

Good observation and we'll take a look but I bet this is a chicken/egg issue since the module name is in the module.xml and we need that to understand which are disabled through config.php.

fooman commented 9 years ago

So I guess one way to solve the chicken/egg problem then would be to first look in config.php and have it as a requirement that the path matches the module name.

app/code/Namespace/Module/etc/module.xml file would only be read if Namespace_Module is enabled.

in other words the following app/code/Namespace/Module/etc/module.xml file

<?xml version="1.0"?>
    <config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../../../lib/internal/Magento/Framework/Module/etc/module.xsd">
    <module name="SomeOtherNamespace_SomeOtherModuleName" version="2.0.0"/>
</config>

would be disallowed. This would work for me but not sure if this change would be worth it. Maybe the result of a failed validation can be made less drastic.

alankent commented 9 years ago

The goal of disabling a module is not to disable badly implemented modules (e.g. bad module.xml file) but rather to turn a module off for a period of time. E.g. a merchandising extension talking to a SaaS service that is down at the moment - turn the module off until its working again. With Composer, if you really don't need a module then remove it from your site completely. Its easy to add it back later.

So we will have a bit of a think, but I suspect I agree its probably not worth changing (compared to other work to be done). Other fish to fry!

antonmakarenko commented 9 years ago

The config.php is created based on what's in module.xml. For not enabled modules, the config.php will have a record in the list with 0 value.

muasir commented 9 years ago

@fooman I removed the bug tag, as this seems more of question than bug. Let's know if your query was answered by @antonmakarenko. If yes, let's close this issue.

antonmakarenko commented 9 years ago

Currently, the methods that request module.xml information are used only in the following places:

  1. In Setup application during installing/upgrading modules
  2. In Magento application runtime in order to check if current registry of modules is corresponding to what’s in the source code. This is performed only once and then cached, and will be performed only after the configuration cache is invalidated.

In the second case, The algorithm can be optimized to avoid those modules that are not enabled. It will be a performance optimization at a cost of more complexity. IMO it is not worth the effort.

fooman commented 9 years ago

I think this can be closed as wontfix with effort not worth the payoff.

vpelipenko commented 9 years ago

Discussion is closed.