0

Current sitution:

If one of you extensions depends on another, that isn't active or not installed it breaks page load and you'll see error report page.

<?xml version="1.0"?>
<config>
    <modules>
        <My_Module>
            <active>true</active>
            <codePool>local</codePool>
            <depends>
                <Non_Existing />
            </depends>
        </My_Module>
    </modules>
</config>

Desired behavoir:

If dependency is missing, just do not load the extension, but load the page correctly.

The related code is this: https://github.com/OpenMage/magento-mirror/blob/magento-1.8/app/code/core/Mage/Core/Model/Config.php#L835-L881

I played around a bit and modified code that it does not load the extension:

/**
 * Sort modules and check depends
 *
 * @param array $modules
 * @return array
 */
protected function _sortModuleDepends($modules)
{
    foreach ($modules as $moduleName => $moduleProps) {
        $unset = false; // added
        $depends = $moduleProps['depends'];
        foreach ($moduleProps['depends'] as $depend => $true) {
            if ($moduleProps['active'] && ((!isset($modules[$depend])) || empty($modules[$depend]['active']))) {
                // replaced Mage::throwException
                Mage::log(
                    Mage::helper('core')->__('Module "%1$s" requires module "%2$s".', $moduleName, $depend)
                );
                // added
                unset($modules[$moduleName]);
                $unset = true;

            }
            if (!$unset) { // added
                $depends = array_merge($depends, $modules[$depend]['depends']);
            }
        }
        if (!$unset) { // added
            $modules[$moduleName]['depends'] = $depends;
        }
    }
    $modules = array_values($modules);

    $size = count($modules) - 1;
    for ($i = $size; $i >= 0; $i--) {
        for ($j = $size; $i < $j; $j--) {
            if (isset($modules[$i]['depends'][$modules[$j]['module']])) {
                $value       = $modules[$i];
                $modules[$i] = $modules[$j];
                $modules[$j] = $value;
            }
        }
    }

    $definedModules = array();
    foreach ($modules as $key => $moduleProp) { // added $key
        $unset = false; // added
        foreach ($moduleProp['depends'] as $dependModule => $true) {
            if (!isset($definedModules[$dependModule])) {
                // replaced Mage::throwException
                Mage::log(
                    Mage::helper('core')->__('Module "%1$s" cannot depend on "%2$s".', $moduleProp['module'], $dependModule)
                );
                // added
                unset($modules[$key]);
                $unset = true;
            }
        }
        if (!$unset) { // added
            $definedModules[$moduleProp['module']] = true;
        }
    }

    return $modules;
}

Question: it seems to work as intended, but did i've missed something or can I use this safly in production?


Update

In addition to Marius answer just some test code ... I created three modules that rewrite Mage_Catalog_Block_Product_View and just outputs a message on getProduct() call.

First one is disabled:

<config>
    <modules>
        <Sr_Disabled>
            <active>false</active>
            <codePool>local</codePool>
        </Sr_Disabled>
    </modules>
</config>

Second one is enabled, but depends on the disabled one:

<config>
    <modules>
        <Sr_Enabled>
            <active>true</active>
            <codePool>local</codePool>
            <depends>
                <Sr_Disabled />
            </depends>
        </Sr_Enabled>
    </modules>
</config>

Third one to test ...

<config>
    <modules>
        <Sr_Test>
            <active>true</active>
            <codePool>local</codePool>
            <!--
            <depends>
                <Sr_Enabled />
            </depends>
            -->
        </Sr_Test>
    </modules>
</config>

I understand your concerns, but I could not find any wrong behavior yet.

For the autoloader you can still check if the module is enabled Mage::helper('core')->isModuleEnabled('Sr_Enabled'), so you can cover the case where module is just disabled, but still present in code pool.

If it's deleted and you want to extends one of its classes you'll get default error message Warning: include(Sr/Enabled/Block/Product/View.php): failed to open stream. Indeed, this would break the site, but this always happens if a class isn't available. This can be solved if you set <depends> node in your module ...


Update 2

For ProxiBlues note ... calling a non existing helper from layout it could break the site ...

  • so either add a check for isModuleEnabled()
  • or edit Mage_Core_Model_Layout::_generateAction

Not tested yet, but it should work ...add a exists/enabled/callable check to:

$args[$key] = call_user_func_array(array(Mage::helper($helperName), $helperMethod), $arg);
4
+50

In magento 1 the point of the dependency is to tell one module that it cannot function without an other. You should not toy with the way the dependencies are checked. The fact that it works may mean that the declared dependency was not actually a dependency. Or that it was a soft dependency.
Maybe the dependency was determined by one class in the module that extends one class in the dependency. In this case it will work because the autoloader does not care about active or inactive modules. It just searches for the class.
Even in this case it is dangerous. Usually you don't keep disabled modules for long. On the next cleanup if you remove the disabled module you may be in trouble.
Conclusion: do not touch the dependency check just for the sake of a module. Change the module instead

  • 2
    Additionally, if I work on a site to which I was not the original creator, or installer of modules, or I do a cleanup of a site, and remove a module, the proposed change will not alert me that I had broken dependencies. So shit will break. Oddities will occur that can take hours to track down. This is why we have dev and staging sites. In production you should never just disable modules, since, well, you can break the site. You want this error, so you know. – ProxiBlue Jun 18 '17 at 16:24
  • Following up on @Marius, the module that you don't load may define something (like a template or feature) which if not loaded would prevent your page from loading anyway. I have had plenty of situations where custom templates for homepage or other custom pages have been contained in one module which when disabled, broke the site. Simply not loading that module may prevent your site from even appearing to load (e.g. white screen instead of error message). – Brett Jun 18 '17 at 19:33
  • @Marius understand it, but didn't get what you mean with "soft dependency". If you have time for, can you please add some dangerous pseudo-code? – sv3n Jun 19 '17 at 10:25
  • @ProxiBlue agreed to that disabled modules should be completly removed, but I had this case where someone just diabled one. You will still see the error in system.log, so you can fix it - but the site would load at least. – sv3n Jun 19 '17 at 10:27
  • 1
    @sv3n you will simply cause other stuff to break yes the page may (big may) load, but functionality may be broken. What if a directive in the given template is accessing a helper in the module you are now removing via this code? You are coding to fix a procedural issue: never to disable modules on a live running site. You should rather spend your effort on fixing procedural processes and developer / client understanding of such. – ProxiBlue Jun 19 '17 at 10:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.