-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Updated Varien_Object::getData()
and added getDataByKey()
& getDataByPath()
#3245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested.
Any updates here? (Still no time to work on it) |
I wouldn't know where to start testing it, it's for sure ok but the getdata/setdata is such a core thing that a minor mistake will break everything and there are many use cases to test. |
Indeed, its really hard to read. (also removed some lines) :( I had testscript, that called
First chart is for I dont have the script anymore ... but you can see for 200.002 calls, the old method is called 200.009 time. Depending on input it has called itself and/or Second table compared ...
Code comes from M2 ... im pretty sure, its save to merge. |
* | ||
* If $index is specified it will assume that attribute data is an array | ||
* and retrieve corresponding member. | ||
* and retrieve corresponding member. If data is the string - it will be explode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If data is a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, im no native speaker, but "the" seem correct here.
Hhowever ... can you please use github suggestion function? (ctrl-g)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording in that context is not exactly appropriate. It is not an error to turn this PR upside down, it can reformulated. As I can live with some of the OM translations, I can live with this wording.
Description (*)
https://magento.stackexchange.com/questions/248180/magento-2-good-practice-to-use-avoid-magic-getters
Idea was to add
getDataByKey()
&getDataByPath()
, but then did some xhprof tests forgetData()
.https://gist.github.com/sreichel/85730194e5e6ad52876700b4b90aafc3
Code comes from M2 ...
ToDo
Replace magic getters (?)
Contribution checklist (*)