Skip to content
This repository has been archived by the owner on Apr 30, 2023. It is now read-only.

Use the Product ID instead of the SKU in the Feed #3

Closed
wants to merge 1 commit into from

Conversation

danbtl
Copy link
Contributor

@danbtl danbtl commented Dec 10, 2020

The pixel uses the product ID for matching events.

See for example: https://github.com/facebookincubator/facebook-for-magento2/blob/791716e68d9fc685e4f550a4ba67f9fe743481b3/Facebook/BusinessExtension/Block/Pixel/ViewContent.php#L14

So the products should also be saved using the ID instead of the SKU.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 10, 2020
@xxu3-sc
Copy link
Contributor

xxu3-sc commented Dec 10, 2020

Hi @danbtl , thanks for your suggestion. We do notice some users prefer Product ID instead of Sku, our plan is to allow user configure it in UI. I will leave it to @zlik

@danbtl
Copy link
Contributor Author

danbtl commented Dec 10, 2020

@xiangminxufsu either way is fine, it just has to be consistent everywhere. Right now the Pixel uses ID and the Feed uses SKU.

@xxu3-sc
Copy link
Contributor

xxu3-sc commented Dec 10, 2020

@xiangminxufsu either way is fine, it just has to be consistent everywhere. Right now the Pixel uses ID and the Feed uses SKU.

Thanks for reminding us this bug, we will change the pixel code soon.

@zlik
Copy link
Contributor

zlik commented Dec 10, 2020

The pixel uses the product ID for matching events.

See for example:

https://github.com/facebookincubator/facebook-for-magento2/blob/791716e68d9fc685e4f550a4ba67f9fe743481b3/Facebook/BusinessExtension/Block/Pixel/ViewContent.php#L14

So the products should also be saved using the ID instead of the SKU.

Thanks for flagging, @danbtl! We definitely need to make the identifier field consistent across the extension. We're thinking to make it configurable. Do you have any preferences which field to use (SKU or ID)?

@danbtl
Copy link
Contributor Author

danbtl commented Dec 10, 2020

Do you have any preferences which field to use (SKU or ID)?

@zlik I would prefer the ID, because it's the primary key in Magento. The SKU can be changed and you could end up with duplicates in that case.

However, merchants usually identify their products by SKU, so it might be helpful for them to see the SKU in the Catalog Manager.

Making it configurable is a good idea.

@zlik
Copy link
Contributor

zlik commented Dec 10, 2020

Do you have any preferences which field to use (SKU or ID)?

@zlik I would prefer the ID, because it's the primary key in Magento. The SKU can be changed and you could end up with duplicates in that case.

However, merchants usually identify their products by SKU, so it might be helpful for them to see the SKU in the Catalog Manager.

Making it configurable is a good idea.

Makes sense, thanks for great feedback! I agree SKU can be changed which will create duplicates. My only concern (I'm sure I'm overreacting) with ID was it is not super robust either – merchant could delete products from the database and create them again a different ID (due to mistake, or some data flow process).

@xxu3-sc
Copy link
Contributor

xxu3-sc commented Dec 11, 2020

I merged a PR based on this

@danbtl
Copy link
Contributor Author

danbtl commented Dec 11, 2020

@xiangminxufsu thanks!

@danbtl danbtl closed this Dec 11, 2020
facebook-github-bot pushed a commit that referenced this pull request Oct 21, 2022
Summary:
This is to avoid Magento exception in Stores -> Configuration when the Magento_InventoryApi module is disabled:

```
report.CRITICAL: Error: Cannot instantiate interface Magento\InventoryApi\Api\StockRepositoryInterface in /var/www/html/ego_240/vendor/mag
ento/framework/ObjectManager/Factory/AbstractFactory.php:121
Stack trace:
#0 /var/www/html/ego_240/vendor/magento/framework/ObjectManager/Factory/Compiled.php(108): Magento\Framework\ObjectManager\Factory\AbstractFactory->createObject()
#1 /var/www/html/ego_240/vendor/magento/framework/ObjectManager/ObjectManager.php(70): Magento\Framework\ObjectManager\Factory\Compiled->create()
#2 /var/www/html/ego_240/app/code/Facebook/BusinessExtension/Model/Config/Source/Product/InventoryStock.php(46): Magento\Framework\ObjectManager\ObjectManager->get()
#3 /var/www/html/ego_240/app/code/Facebook/BusinessExtension/Model/Config/Source/Product/InventoryStock.php(57): Facebook\BusinessExtension\Model\Config\Source\Product\InventoryStock->getStockRepository()
#4 /var/www/html/ego_240/vendor/magento/module-eav/Model/Entity/Attribute/Source/AbstractSource.php(168): Facebook\BusinessExtension\Model\Config\Source\Product\InventoryStock->getAllOptions()
#5 /var/www/html/ego_240/vendor/magento/module-config/Model/Config/Structure/Element/Field.php(461): Magento\Eav\Model\Entity\Attribute\Source\AbstractSource->toOptionArray()
```

Reviewed By: fbisaso

Differential Revision: D32070111

fbshipit-source-id: 103428392bc445e544d1b302a4e49296eba4628b
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants