#18624 Refactoring: Extract addLinks to own class

[amenk]

WIP

follow up of #21230

Aim is to refactor product links during import, with the final aim of an extension point to import own relation types

[magento-engcom-team]

Hi @amenk. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[amenk]

Still WIP, but want to kick of the CI for some full tests

[amenk]

Todo

[ ] Inject LinkProcessor via DI (I think we need a factory)
[ ] Maybe optimized passed classes (help-wanted)
Currently it's quite stupid:

     $linkProcessor = new LinkProcessor( // FIXME: instantiate via DI (but there are som
            $this,
            $this->_connection,
            $this->_linkFactory,
            $this->_resourceHelper,
            $this->_dataSourceModel,
            $this->skuProcessor,
            $this->_logger,
            $this->getProductEntityLinkField()
        );

[amenk]

• integration tests fail of strange reasons (not sure if related to my change or bug in the CI server)
• di.xml config is not optimal -- it would be cool to use init_parameter but it does not seem to be allowed in arrays, who to do?

[rogyar]

Hi @amenk. Regarding the integration tests, take a look at:

https://github.com/magento/magento2/pull/21658/files#diff-355bef4df35549f0f67776426f504636R208

I believe, there's no isSkuExist method within the scope of this class.

[amenk]

@dmanners Thanks for your review.

[sivaschenko]

Thanks for the pull request @amenk ! Please see my code review comments

[amenk]

Now a conflict appeared :-( Have to review this.

is this otherwise ready to merge?

[amenk]

Conflicts solved -- was only static tests suppression

[amenk]

@sivaschenko Which of the failing builds do I have to tackle?

• Sample Data Tests build does not provide a report
• Semantic Version Checker: suggests a MINOR bump, which should be fine, right?
• Magento Health Index: Should I tackle that protected variable message?

[amenk]

@sivaschenko Which of the failing builds do I have to tackle?

• Sample Data Tests build does not provide a report
• Semantic Version Checker: suggests a MINOR bump, which should be fine, right?
• Magento Health Index: Should I tackle that protected variable message?

And what about the static tests?

When I run LiveCodeTest locally I do not get those errors: (see https://testing-service.magento-community.engineering/reports/magento/magento2/pull/21658/7df8ac0b-32e5-4377-aebc-7dbf531faa3c/6412/Statics/allure-report-ce/index.html#suites/51d3ab4d5d77a0bc6a7ea344e9e7e6be/5823c27e50eef3c2/)

Is this test not idempotent?

[amenk]

@odubovyk also worked on the refactoring of this part - which is great... I think I have to start over with extracting the code to a seperate class. It is very demotivating, that my pull request did not receive attention for almost weeks

[amenk]

@odubovyk Would you be willing to extract the link processing code, to a separate classes as well as allowing injecting additional link types?

[DanielRuf]

It is very demotivating, that my pull request did not receive attention for almost weeks

Sorry to hear this but most of us do this here only in their free time as hobby and there are many open PRs and issues - there is just much to do.

[amenk]

It is very demotivating, that my pull request did not receive attention for almost weeks

Sorry to hear this but most of us do this here only in their free time as hobby and there are many open PRs and issues - there is just much to do.

yes, agreed, me, too :-) Already trying to get motivated again ... I hope I can work with @odubovyk together to extract his code to a separate class to get the link-types injected. His work anyways seems to have some additional improvements to this part of the code (for example reducing cyclomatic-complexity), but it's all kept in the original Product-"God"-Class

[sivaschenko]

Hi @amenk , thanks for your patience! Sorry, I might have skipped reviewing this PR as I was probably confused by "WIP" in the first line of description. A great refactoring has been done here!

As for tests:

• An optional parameter added to the constructor has to be approved by @akaplya
• The access level of protected property reported by Magento Health Index should be changed to private
• The cyclomatic complexity reported by static tests should be reduced

I am also concerned by introducing new public methods to app/code/Magento/CatalogImportExport/Model/Import/Product.php as that's an API class and such changes are not backward compatible. Is it possible to avoid this change

[amenk]

Thank you. Actually there are major merge conflicts now and the process of refactoring might have to be done again. The good thing is that things like cyclomatic complexity were already fixed there. Have to check again.

[amenk]

I am trying to re-do the PR currently.

[amenk]

follow up PR:

#23191

@sivaschenko @DanielRuf @dmanners

[m2-assistant]

Hi @amenk, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.