Performance issued due too old_fields_map functionality

[tmotyl]

Magento 1.6 has introduced a _initOldFieldsMap functionality in VarienObject.
It was used to keep backward compatibility with code written pre 1.6.
As in 1.6 some db fields names has been changed.
In some models the _initOldFieldsMap returns static array (like in stock), in others like product it gets configuration from xml.

There are 3 solutions:

1. either remove the functionality completely which would be breaking fro pre 1.6 code
2. instead of checking configuration just return hard coded mapping in _initOldFieldsMap (this will be non breaking for pre v1.6 code, but would not allow people to add/change mapping through xml)
3. create a static runtime cache in every odel, so configuration is read only once

I would prefer solution 1)

See: https://github.com/lizards-and-pumpkins/magento-connector/issues/143

I've spotted the issue when profiling product sales report.
And saw that on every prodct creation Magento is asking configuration for some node, creating tons of objects which are discarded right after and this triggers PHP garbage collection.

[kiatng]

I agree with option 1, remove it completely.

[Flyingmana]

removing it completely is a serious BC break. (we also do not know, if people did misuse it in some way)

We will probably not include it in Version 19.

The process for serious BC breaks should in best case go over our RFC process, to give all participating parties enough time to comment on it.
https://github.com/OpenMage/rfcs

solution 3. seams to be a reasonable short term solution which can be accepted in our current version 19.

I absolute support Solution 1, but it has to go over the RFC process.
And should probably suggest a deprecated message, so people notice they are using it.
Therefore we should in parallel take Solution 3.

[tmotyl]

@Flyingmana
I think that instead of trying to be non breaking at all cause, I would say we should do breaking changes but document them.
I think we should say "we don't support pre 1.6 extensions code". Magento 1.6 was released Nov 2009, so it was plenty of time for people to adopt the new API.
If people will complain, we can always add a compatibility layer as an external module.
We all have limited resources and I think we should spend it on making improvements rather than maintaining (for free) old code which maybe nobody uses.

Feel free to convert this issue and attached PR as RFC. My objective in next weeks is rather solving issues and perfromance optimizations so I will keep pushing new PR's to the LTS repo.

Btw, when I did profiling with all my changes combined, I've got 90% speed improvement in the product sold report on both memory usage and cpu.

[kiatng]

The old_field_map is a good name because it is old. Its sole function is to map the $key in getData or in magic getter and setter from the old field names which no one uses, except some old extensions years ago. For these old extensions that still use the old names, if they exist at all, would not be so difficult to update to new names.

[colinmollenhour]

It does seem very unlikely that maintaining support for these few very old field names is very important. I'd like to get rid of this unnecessary complexity in Varien_Object since it is used so widely. So, for a slightly better performance boost across all code we could while still maintaining most of the BC we could remove the _oldFieldsMap support entirely by just adding getters and setters for these specific fields. E.g.:

    public function getBaseWeeeTaxAppliedRowAmnt()
    {
        return $this->getData('base_weee_tax_applied_row_amount');
    }

    public function setBaseWeeeTaxAppliedRowAmnt($value)
    {
        $this->setData('base_weee_tax_applied_row_amount', $value);
        return $this;
    }

[tmotyl]

I like your Idea @colinmollenhour

If we want we could handle deprecations;

1. add trigger deprecation error in these places.
   trigger_error('Method is deprecated and will be removed in vXXX, use new method XXX.', E_USER_DEPRECATED);
   E_USER_DEPRECATED is supported since PHP 5.3.0

2. Then in error handler we can catch these errors and log to separate log file deprecation.log.
   I can provide code examples how other systems like TYPO3 CMS are handling that nicely.

3. document/communicate that we will remove stuff in next version

4. remove old stuff and release new version

[colinmollenhour]

Deprecation notice in logs is a good idea, what about simply

Mage::log('....', Zend_Log:WARN, 'deprecation.log');

[tmotyl]

Also ok

[kiatng]

The old_field_map is a good name because it is old. Its sole function is to map the $key in getData or in magic getter and setter from the old field names which no one uses, except some old extensions years ago. For these old extensions that still use the old names, if they exist at all, would not be so difficult to update to new names.

I was wrong about its sole function. Today, I have a use case for protected function _initOldFieldsMap() and $this->_oldFieldsMap in a model class. I needed to change a column from old name to new name as well as its type, and add a FK. $_oldFieldsMap provides a quick fix without worrying about breaking the old references in the code. This provides a way to quickly roll out new features in production and then if necessary, go back later to correct the old references.

[tmotyl]

I think the biggest performance gain comes from avoiding to fetch the fields mapping from configuration on every object creation.
If the config was hardcoded in the class we will get a performance boost and keep most of the functionality.

[fballiano]

Closing since #921 was merged