Refactor: Fix lack of customizability of the protected variable linkN… #21230
Conversation
|
Hi @amenk. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
|
Looks good |
| * | ||
| * @return array | ||
| */ | ||
| public function getLinkNameToId() |
There was a problem hiding this comment.
Breaking class encapsulation is not a good idea. As this class is pretty legacy I believe the recommended way would be to extend it.
Another possible way is to inject such data via constructor but I'm not sure it makes sense to do such refactoring.
There was a problem hiding this comment.
@orlangur thanks for looking into this.
I see breaking class encapsulation as a small problem, because we only add a getter, not a setter.
Extending and rewriting the class would not work well with multiple custom modules (in my case the MagePal module needs to put in the link name; another module we use - FireBear Import Export - already rewrites the class)
Constructor injection might work, but as you say the class seems legacy and I am not sure how to do that. Feel free to propose an alternative way.
There was a problem hiding this comment.
Another issue with getter is that plugins will be called each time it is called. So, adding such getter is not a proper extension point.
I was suggesting adding a parameter so that links can be specified later via di.xml. This wouldn't improve situation a lot actually.
Instead of this, please move links manipulation logic into a new class.
There was a problem hiding this comment.
I was just following what is done in the class already -- there is also public getAttrSetIdToName which works the same.
But I get the argument that it is not a perfect extension point -- but at least it's a lightweight one.
So shall I refactor the whole links manipulation into a new class now? I was just hoping to make the situation a bit better, and am not sure if I am able to invest lots of time here.
What do you think about merging this one and put a refactoring backlog entry for 2.3.x ? I don't think it makes much sense to do such refactoring in 2.2.x ? Would that be possible?
There was a problem hiding this comment.
Maybe the larger scale refactoring can be something I could tackle at the MageFest hackathon :-)
There was a problem hiding this comment.
@amenk I'll consult with other maintainers in Slack whether such getters addition is something we can live with.
March 5-8th, 2019, not too much time to wait 👍
So shall I refactor the whole links manipulation into a new class now?
Yes, I was thinking of this, most part is in _saveLinks but maybe there are some others.
I don't think it makes much sense to do such refactoring in 2.2.x ? Would that be possible?
Sorry, I missed this PR is targeting 2.2. Currently all changes must be delivered to 2.3.x first, then in case of backward-incompatible refactoring of 2.3 we may think of trade-offs.
There was a problem hiding this comment.
It's not easy to contribute :-D
| * | ||
| * @return array | ||
| */ | ||
| public function getLinkNameToId() |
There was a problem hiding this comment.
Another issue with getter is that plugins will be called each time it is called. So, adding such getter is not a proper extension point.
I was suggesting adding a parameter so that links can be specified later via di.xml. This wouldn't improve situation a lot actually.
Instead of this, please move links manipulation logic into a new class.
|
I am closing this one and will make a new PR against 2.3-develop |
|
Hi @amenk, thank you for your contribution! |
|
@orlangur As for the refactoring --- will we bump the major module version or do I have to make everything backwards compatible ? |
|
@amenk I believe it's bumped automatically during release. |
Description (*)
Currently it is not possible to add own custom links to the import
Fixed Issues (if relevant)
See magepal/magento2-link-product#2 (comment)
Manual testing scenarios (*)
No need to test, it's a trivial change by adding an extension point where no was before
Contribution checklist (*)