Skip to content

Refactor: Fix lack of customizability of the protected variable linkN… #21230

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Refactor: Fix lack of customizability of the protected variable linkN… #21230

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions app/code/Magento/CatalogImportExport/Model/Import/Product.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,7 @@ protected function _saveLinks()
$nextLinkId = $this->_resourceHelper->getNextAutoincrement($mainTable);

// pre-load 'position' attributes ID for each link type once
foreach ($this->_linkNameToId as $linkName => $linkId) {
foreach ($this->getLinkNameToId() as $linkName => $linkId) {
$select = $this->_connection->select()->from(
$resource->getTable('catalog_product_link_attribute'),
['id' => 'product_link_attribute_id']
Expand Down Expand Up @@ -1237,7 +1237,7 @@ protected function _saveLinks()
$linkKey = "{$productId}-{$linkData['linked_id']}-{$linkData['link_type_id']}";
$productLinkKeys[$linkKey] = $linkData['id'];
}
foreach ($this->_linkNameToId as $linkName => $linkId) {
foreach ($this->getLinkNameToId() as $linkName => $linkId) {
$productIds[] = $productId;
if (isset($rowData[$linkName . 'sku'])) {
$linkSkus = explode($this->getMultipleValueSeparator(), $rowData[$linkName . 'sku']);
Expand Down Expand Up @@ -2274,6 +2274,16 @@ public function getAttrSetIdToName()
return $this->_attrSetIdToName;
}

/**
* Attribute set ID-to-name pairs getter.
*
* @return array
*/
public function getLinkNameToId()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the larger scale refactoring can be something I could tackle at the MageFest hackathon :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not easy to contribute :-D

{
return $this->_linkNameToId;
}

/**
* DB connection getter.
*
Expand Down