Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dont show disabled modules in "Disable Modules Output" config #4305

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Oct 24, 2024

Description (*)

Don't show disabled modules in admin - system - config - advanced - advanced

Manual testing scenarios (*)

  1. disable a module
  2. flush cache
  3. go to admin - system - config - advanced - advanced

Disabled module is still there.

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Oct 24, 2024
@addison74 addison74 changed the title Dont show disbaled modules in "Disable Modules Output" config Dont show disabled modules in "Disable Modules Output" config Oct 24, 2024
@addison74 addison74 linked an issue Oct 24, 2024 that may be closed by this pull request
addison74
addison74 previously approved these changes Oct 24, 2024
Copy link
Contributor

@addison74 addison74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it works as expected.

@sreichel sreichel marked this pull request as draft October 25, 2024 01:30
@kiatng
Copy link
Contributor

kiatng commented Oct 25, 2024

Instead of not showing, is it better to inform the users that the module is inactivated in the global config? An example:
image

@sreichel
Copy link
Contributor Author

Thought about it, but it makes no sense.

  • it is only displayed if it is still there in filesystem
  • it is disabled. Its confusing.

I put it on draft b/c ...

  • pass the modules array to observer (not only array keys)
  • add observer
  • hide disabled modules there

@sreichel sreichel marked this pull request as ready for review October 25, 2024 12:43
@sreichel sreichel marked this pull request as draft October 25, 2024 12:44
@sreichel sreichel marked this pull request as ready for review October 27, 2024 18:48
@sreichel
Copy link
Contributor Author

sreichel commented Nov 7, 2024

Instead of not showing, is it better to inform the users that the module is inactivated in the global config? An example: image

I tried, but it would require some more changes.

@kiatng
Copy link
Contributor

kiatng commented Nov 10, 2024

Alternative:

   // File app/code/core/Mage/Adminhtml/Block/System/Config/Form/Fieldset/Modules/DisableOutput.php
    protected function _getFieldHtml($fieldset, $moduleName)
    {
        $isInactive = (string) Mage::getConfig()->getNode('modules')->$moduleName->active !== 'true';
        // ...
        $field = $fieldset->addField(
            $moduleName,
            'select',
            [
                'name'          => 'groups[modules_disable_output][fields][' . $moduleName . '][value]',
                // ...
                'disabled'      => $isInactive,
                'title'         => $isInactive ? 'Inactivated in /etc/modules' : '',
        //...

@sreichel
Copy link
Contributor Author

Wouldnt this not bypass the observers?

@kiatng
Copy link
Contributor

kiatng commented Nov 11, 2024

Wouldnt this not bypass the observers?

Yes, observer is not required. And if you want to not show the modules: if ($isInactive) return ''. The is active check is microseconds. It's a simpler implementation, no additional loop on modules.

@sreichel
Copy link
Contributor Author

The is active check is microseconds.

I've tested it. Running Mage::getConfig()->getNode('modules') 60 times is far slower.

@kiatng
Copy link
Contributor

kiatng commented Nov 12, 2024

$moduleName = 'Mage_Core';
$isInactive = (string) Mage::getConfig()->getNode('modules')->$moduleName->active !== 'true';

My tests:

  1. with all cache disabled, 5 microseconds
  2. with all cache enabled, 8 microseconds, yes it's worst!

Really weird. Is it just me or for you too?

@justinbeaty
Copy link
Contributor

$moduleName = 'Mage_Core';
$isInactive = (string) Mage::getConfig()->getNode('modules')->$moduleName->active !== 'true';

My tests:

  1. with all cache disabled, 5 microseconds
  2. with all cache enabled, 8 microseconds, yes it's worst!

Really weird. Is it just me or for you too?

I actually don’t think the modules xml section is cached at all, probably so you can disable a broken module. But I’m not at my desk to confirm this.

@kiatng
Copy link
Contributor

kiatng commented Nov 12, 2024

Another test:

        $modules = array_keys(Mage::getConfig()->getNode('modules')->asArray()); // 88 elements
        foreach ($modules as $moduleName) {
            $result[$moduleName] = (string) Mage::getConfig()->getNode('modules')->$moduleName->active !== 'true';
        }
  1. Config cache disabled, 650 microseconds
  2. Config cache enabled, 930 microseconds, it's better to disable cache?

@kiatng kiatng closed this Nov 12, 2024
@kiatng kiatng reopened this Nov 12, 2024
@sreichel
Copy link
Contributor Author

My tests:

require_once('app/Mage.php');
umask(0);
Mage::app()->loadArea('adminhtml'); #to call the observer

function test4305_Observer() {
    $test = new Mage_Adminhtml_Block_System_Config_Form_Fieldset_Modules_DisableOutput();
    $test->getModules();
}

function test4305_Config() {
    for ($i = 0; $i <= 60; $i++) {
        $isInactive = (string)Mage::getConfig()->getNode('modules')->Mage_Core->active !== 'true';
    }
}

  • Config cache disabled, 650 microseconds

  • Config cache enabled, 930 microseconds, it's better to disable cache?

No.

Config cache enabled (last 5) vs config cache disabled.

xh_cache1

I'd recommand to test #4325.

@sreichel sreichel requested review from kiatng and addison74 November 25, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml phpunit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabled modules in admin config "Disable Modules Output"
4 participants