-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Backward compatibility provided by #19765 #22761
Comments
Hi @Nazar65. Thank you for your report.
Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:
For more details, please, review the Magento Contributor Assistant documentation. @Nazar65 do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?
|
Thank you @Nazar65 for proposing this as |
Hi @Nazar65. Thank you for working on this issue.
|
Hi @Nazar65. Thank you for working on this issue.
|
Hi @engcom-Charlie. Thank you for working on this issue.
|
✅ Confirmed by @engcom-Charlie Issue Available: @engcom-Charlie, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself. |
The change was done more than a year ago and introducing other changes now may cause other backwards-incompatible changes. |
Summary (*)
#19765
#19761 (comment)
Current fixes still don't provide complete backward compatibility with code designed for Magento 2.0.X, 2.1.X, 2.2.X.
IMO it is not good at all to have any variables related to SKU in abstract entity code. In the best case new class must be implemented and extended from abstract entity.
In case it is impossible to achieve that, then it would be probably better to introduce one more variable to the constructor. It could be something like
Examples (*)
Current fix
$this->_processedEntitiesCount = (count($skuSet)) ? : $this->_processedRowsCount;
can also break custom import. At least it breaks my import, which was written basing on Magento\CatalogImportExport\Model\Import\Product where $this->_processedEntitiesCount is calculated within validateRow() method. validateRow() is triggered from _saveValidatedBunches(), which means that
$this->_processedEntitiesCount = (count($skuSet)) ? : $this->_processedRowsCount;
would override everything after all validateRow(). Similar as it is in M 2.3 for Magento\CatalogImportExport\Model\Import\Product. The only difference that 'sku' is present in CSV for product import and
$this->_processedEntitiesCount = (count($skuSet)) ? : $this->_processedRowsCount;
is valid in this case. But for custom imports written earlier $this->_processedEntitiesCount had to be calculated in the custom import model extended from Magento\ImportExport\Model\Import\Entity\AbstractEntity .
Proposed solution
Then 'sku' could be injected for all core import classes via di.xml.
In _saveValidatedBunches() we could rename $skuSet into $uniqueFieldSet.
In case it is impossible to go with null as default value for $uniqueField, then we could set 'sku' there,
but then in custom code all developers would have to override that 'uniqueField' in di.xml to make custom import code operational again.
The text was updated successfully, but these errors were encountered: