bugfix + add module names to helper

[sreichel]

Description (*)

Read: https://magento.stackexchange.com/questions/46255/ecomdev-phpunit-translation-not-working-with-mocked-helper/46256#46256

• 1st commit: added module name to all helpers
• 2nd: fix for Imagine what bugs a module name starting with "Helper" would cause...

<?php
$class = 'Vendor_Helper_Helper_Helper_Test';

$old = substr($class, 0, strpos($class, '_Helper'));
$new = implode('_', array_slice(explode ('_', $class), 0, 2));

var_dump($old);
var_dump($new);

To test you can set a debug breakpoint at Mage_Core_Helper_Abstract::_getModuleName()

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)

[elidrissidev]

I understand the bug fix, but is it really necessary to set the module name explicitly in every helper?

[fballiano]

I understand the bug fix, but is it really necessary to set the module name explicitly in every helper?

exactly what I was thinking :-(

[sreichel]

I think it is bit faster to get module name from property, instead of retrieve it from classname.

[sreichel]

This method is called on nearly every page for at least ten times (or more often) ...

protected function _getModuleName()
{
    if (!$this->_moduleName) {
        $class = get_class($this);
        $this->_moduleName = substr($class, 0, strpos($class, '_Helper'));
    }
    return $this->_moduleName;
}

Why to not set $_moduleName to skip that useless calls?

[kiatng]

Why I agree with @sreichel:

magento-lts/app/code/core/Mage/Core/Helper/Abstract.php

Line 35 in fb12d0f

protected $_moduleName;

3 reasons:

1. It's available for the child helpers
2. It's harmless for the children to make use of it
3. It saves many computing cycle times

[Flyingmana]

Why to not set $_moduleName to skip that useless calls?

because most helper classes should not need to know their module name.
It could be seen as a violation of separation of concerns.
Especially as there was a best practice to only use the translate method from the data helper I think?

It also goes further away on how the Block classes do it, which do it fully dynamically.

anyway, this optimization saves around 0.02ms per request according to my abstracted benchmark, if all of these classes are loaded and used.

[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 9608e10. ± Comparison against base commit 47c8177.