Config cache for system.xml loader: Speed improvement

[lamskoy]

Description (*)

System.xml loading is not cached anywhere. So each time you open backend page in Magento it scans ALL system.xml inside all modules, merges them and doesn't cache this big XML structure.

Header menu is rendered from system.xml contents too, not only config fields and sections!

My PR implements caching of this stuff.

It decreases any admin page load time in about 1.5 sec on second load on my local machine :)

Manual testing scenarios (*)

Open any magento config page twice and measure time of page load

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[luigifab]

For me:

• before / after
• From 550/650 ms to 350/450 ms at localhost with PHP 7.4 when only CONFIG cache is enabled.
• From ~1700 ms to 1350/1450 ms at localhost with PHP 7.4 when all cache are disabled.

[luigifab]

I think that this error is due to this PR (exception.log):

Exception: "$string" parameter for simplexml_load_string is not a string in
  lib/Varien/Simplexml/Config.php:517

[Flyingmana]

From ~1700 ms to 1350/1450 ms at localhost with PHP 7.4 when all cache are disabled.

sounds like there is still a check missing, to not cache it when the cache is disabled?

[elidrissidev]

Agree with @Flyingmana, I don't see any code that checks if the cache is enabled. Example from Mage_Api_Model_Config:

magento-lts/app/code/core/Mage/Api/Model/Config.php

Lines 58 to 70 in 4e7a8e1

	if (Mage::app()->useCache('config_api')) {
	if ($this->loadCache()) {
	return $this;
	}
	}
	$config = Mage::getConfig()->loadModulesConfiguration('api.xml');
	$this->setXml($config->getNode('api'));
	if (Mage::app()->useCache('config_api')) {
	$this->saveCache();
	}
	return $this;

[lamskoy]

Agree with @Flyingmana, I don't see any code that checks if the cache is enabled. Example from Mage_Api_Model_Config:

magento-lts/app/code/core/Mage/Api/Model/Config.php

Lines 58 to 70 in 4e7a8e1

	if (Mage::app()->useCache('config_api')) {
	if ($this->loadCache()) {
	return $this;
	}
	}
	$config = Mage::getConfig()->loadModulesConfiguration('api.xml');
	$this->setXml($config->getNode('api'));
	if (Mage::app()->useCache('config_api')) {
	$this->saveCache();
	}
	return $this;

Yeah, you're right. Pushed new commit with fix. Bound to 'config' cache

[elidrissidev]

Since you're including it with the CONFIG tag, can you also update the description to include system.xml? Something like:

-System(config.xml, local.xml) and modules configuration files(config.xml).
+System(config.xml, local.xml) and modules configuration files(config.xml, system.xml).

[lamskoy]

Done with description
"System(config.xml, local.xml) and modules configuration files(config.xml, system.xml)"

[luigifab]

I still get the following error (I didn't searched why - PHP 8.0): Exception: "$string" parameter for simplexml_load_string is not a string in lib/Varien/Simplexml/Config.php:517
After clearing cache and after refresh any configuration pages.

[tmotyl]

I think this chnage deservces an comment in readme/changelog, as it changes an expected behaviour developers have -> now they will need to clear the cache after changing system.xml

[fballiano]

is phpstan right about this?

[elidrissidev]

Yes, because that parameter is not passed to the parent class.

code was updated

[elidrissidev]

Two more changes to please phpstan

[elidrissidev]

@fballiano What do you think about mentioning this change in README under "Between Magento 1.9.4.5 and OpenMage 19.x" as tmotyl suggested?

[fballiano]

@elidrissidev added info to README ;-)

[elidrissidev]

Tested once again and everything works well for me, and also a big improvement in loading speed across the Admin area!

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 14cf6fa. ± Comparison against base commit 417a20d.