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

Cache isModuleOutputEnabled or isModuleEnabled status #4323

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

Conversation

sreichel
Copy link
Contributor

Description (*)

If isModuleOutputEnabled or isModuleEnabled is called multiple times for the same Module, get config once.

Related Pull Requests

  1. see Fix Mage_Adminhtml_Block_Template::isOutputEnabled() for invalid module #4320

Manual testing scenarios (*)

  1. add xdebug breapoints to isModuleOutputEnabled()
  2. ...
    var_dump($test->isModuleOutputEnabled('Mage_Catalog'));
    var_dump($test->isModuleOutputEnabled('Mage_Catalog'));
    var_dump($test->isModuleOutputEnabled('Mage_Catalog'));
    var_dump($test->isModuleOutputEnabled('a'));
    var_dump($test->isModuleOutputEnabled('a'));
    var_dump($test->isModuleOutputEnabled('b'));

# Conflicts:
#	app/code/core/Mage/Core/Helper/Abstract.php
@github-actions github-actions bot added the Component: Core Relates to Mage_Core label Oct 30, 2024
@justinbeaty
Copy link
Contributor

XML / DB config is already cached, how much time is this actually saving?

@sreichel
Copy link
Contributor Author

xh1

For the example above,

@justinbeaty
Copy link
Contributor

I just tested and confirmed it's faster, although only on the order of microseconds per call. I suppose the SimpleXMLElement config cache just has that much overhead.

However, it is not caching per store even though the "Disable Modules Output" can be configured at the store scope. This may cause problems in the app emulation used for sales emails.

I enabled in app/etc/modules/Cm_RedisSession.xml, and disabled the module output for store id 2. Expected to get true / false but got true / true.

<?php

define('MAGENTO_ROOT', getcwd());
require MAGENTO_ROOT . '/app/bootstrap.php';
require_once MAGENTO_ROOT . '/app/Mage.php';

Mage::init();

$test = Mage::helper('core');

var_dump(
    $test->isModuleOutputEnabled('Cm_RedisSession')
);

Mage::app()->setCurrentStore(2);

var_dump(
    $test->isModuleOutputEnabled('Cm_RedisSession')
);

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Oct 31, 2024
@sreichel
Copy link
Contributor Author

sreichel commented Oct 31, 2024

even though the "Disable Modules Output" can be configured at the store scope

I did not see that and did not test it. Thanks.

Config is alerady cached, so it brings no benfits. Check if module is enabled still save some calls. (seems even faster ... numbers from ddev + xhprof + xhgui)

xh2

@github-actions github-actions bot added Component: Catalog Relates to Mage_Catalog Component: Checkout Relates to Mage_Checkout Component: Customer Relates to Mage_Customer Component: Eav Relates to Mage_Eav Component: Rating Relates to Mage_Rating Component: Bundle Relates to Mage_Bundle labels Oct 31, 2024
@github-actions github-actions bot added Component: Api2 Relates to Mage_Api2 phpstan labels Oct 31, 2024
@sreichel
Copy link
Contributor Author

sreichel commented Oct 31, 2024

#4320 (comment)

However, I only see two calls to this method

Updated:

  • moved methods to parent abstract class ... so its also available for frontend block classes too
    • have access to these methods from everywhere
    • easier to mock in tests
  • added trait to provide same funtionality to models, resource,-models, ... (instead of adding helper methods multiple times)

@github-actions github-actions bot removed the Component: Api2 Relates to Mage_Api2 label Nov 7, 2024
kiatng
kiatng previously approved these changes Nov 10, 2024
@github-actions github-actions bot added Component: Sales Relates to Mage_Sales Component: Rss Relates to Mage_Rss labels Nov 12, 2024
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 Component: Bundle Relates to Mage_Bundle Component: Catalog Relates to Mage_Catalog Component: Checkout Relates to Mage_Checkout Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer Component: Eav Relates to Mage_Eav Component: Rating Relates to Mage_Rating Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales phpstan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants