Make oldFields mapping hardcoded to speed up system

[tmotyl]

The mapping came with Magneto 1.6 to provide a backward compatibility
for old (pre 1.6) db field names.
Now the mapping is hardcoded in the class instead of saved
in the configuration.
This improves performance as Magento will not create tons of objects for
every new product/order/... entities.

Related: #920

[tmotyl]

@colinmollenhour can you take a look at the issue #920
I've described there 3 options how to solve it.
To be honest I would love to start removing deprecated stuff from pre 1.6 era

[Flyingmana]

as mentioned in the related Issue, this is a serious BC break and should follow a certain process therefore.

[tmotyl]

I agree it's breaking (no configuration option any more) but I would not call it serious BC. I have not seen a project or module which would use this configuration options.
But anyway feel free to make RFC out of it, you will have my vote ;)
I will not have time in the next weeks to proceed with formal RFC. But I will continue to push any new fixed I will find of course.

[tmotyl]

FYI, there are few places more with getOldFieldMap:
order address
order creditmemo
order invoice
quote
quote address
quote item

[sreichel]

as mentioned in the related Issue, this is a serious BC break and should follow a certain process therefore.

If its about 11Y old extensions ... just drop it. (IMHO)

I guess with raising composer requirments to PHP7 we broke far more (users/agencies, that still rely on 5.x). If removal is a no-go I'd agree to hardcoded values ..

[tmotyl]

@Flyingmana Can we agree on following:
for branch 1.9 I will make the values hardcoded inside _initOldFieldsMap functions, and for 20.x I will remove the method ?

The base branch was changed.

[tmotyl]

@Flyingmana @fballiano This patch is now less breaking:

• no methods are removed
• mapping which was present in magento is still working

However adding new through configuration will not work ($this->_oldFieldsMap = Mage::helper('sales')->getOldFieldMap).
This mechanism of fetching config on every object creation was the biggest performance bottleneck.

I'm ok with merging this patch to 1.9 branch as it's breaking potential is very limited, but If you prefer to have it only in v20, then lets' be it :)

[luigifab]

I vote for: drop them all, everywhere.

[fballiano]

pre 1.6? let's kill them already!

[tmotyl]

@luigifab @fballiano you mean to merge it to 1.9 or to 20?

[fballiano]

I think @luigifab and me wanted to say that if it's such an old functionality maybe we could remove it?

[tmotyl]

@fballiano We can remove it in separate patch targetted for 20.x only. Not to make unnecessary upgrade hassle for merchants on 1.9 (which we declare backward compatible).
Please lets agree on what to do with this patch in its current form -> merge to 1.9 or to 20.?

[tmotyl]

Basically this patch makes some default Magento reports usable again with more data, so I consider it a good tradeoff for 1.9

[fballiano]

I don't know, since it was discussed to have it in v20 I've no idea what to do then :-D

[tmotyl]

Will I have your vote if we keep it for 20 only?
If yes then please vote and let's merge the thing :) lets not overthink it ;)
We can then promote v20 as much faster in some areas ;)

[fballiano]

I trust that if you say it's a good thing it is for sure, although I keep thinking that we should remove the funcionality completely from v20, for sure there's no pre1.6 code out there anymore. and if there is... this is the last of their problems :-D

[addison74]

As far as I understand it would not be wanted in OM-19 because it would create (possible) trouble with a source code before Magento version 1.6, especially extensions. That means a very long time ago. Who else uses extensions before M1-1.6 that weren't updated later? Hard to say.

I personally think that if the PR is a real improvement to go both versions and if it will create issues we will find out then. It can be quickly reverted for any version and is only a commit. I am also for OM-19 (even I don't use this version in production, OM-20 is concrete).

[colinmollenhour]

I propose we leave it alone for v19 (the PR is already based on v20 so we're good) and add a note to the README.md section of change between v19 and v20 along the lines of:

Removed support for global/sales/old_fields_map defined in XML. #921

Then this can be merged and this PR will be listed in the release notes so users of v20 should do a quick check of their code for old_fields_map to see if they need to make any changes (highly unlikely afaik).

[addison74]

I agree with the opinion expressed by @colinmollenhour.

[fballiano]

@tmotyl I can't add the note to the README since you locked the PR from modifications :-)

[tmotyl]

@fballiano thats strange... consulting github documentation ... seems you can only enable "Allow edits from maintainers" on personal forks and not on organizational forks... see https://github.com/orgs/github-community/discussions/5634
So please let me know what you want to add or lets add the note in the followup PR

[fballiano]

I just wanted to add this line:

Removed support for global/sales/old_fields_map defined in XML. #921

to the README in the "differences between v19 and v20" as suggested by Colin ;-)

[tmotyl]

here you go @fballiano

not so breaking any more

[elidrissidev]

LGTM

[github-actions]

Unit Test Results

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

Results for commit bafc48f.