Skip to content

MSI compatibility while being backward compatible #1739

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

thomas-kl1
Copy link
Contributor

@thomas-kl1 thomas-kl1 commented Apr 17, 2025

Summary

These changes fix the MSI compatibility. It also make this module unnecessary: https://github.com/algolia/algoliasearch-inventory-magento-2

algolia/algoliasearch-inventory-magento-2 is not really maintained anymore and many flaws exists as it does not really follow the changes introduced in the main module.
I've also noticed dead code.

The $product->getIsSalable() method use the alias added by MSI while it also fallback to simple inventory management when MSI is disabled.

I've also updated how the stock_qty is retrieved so it used the method that MSI have plugged, so if MSI is used, it's automatically compatible.

EDIT:

Change in ProductHelper is BIC

@thomas-kl1 thomas-kl1 closed this Apr 17, 2025
@thomas-kl1 thomas-kl1 reopened this Apr 17, 2025
@cammonro
Copy link
Contributor

Thank you for your contribution @thomas-kl1 ! It has been on the roadmap to retire the need for a separate module for MSI since it has long been the default in Magento 2.4. We'll be sure to review and incorporate your proposed changes in an upcoming release!

@cammonro
Copy link
Contributor

This is a quality contribution @thomas-kl1 ! Thank you for taking the time to share this.

We have a different intention regarding the core module but while we address that separately your ideas have been incorporated in algolia/algoliasearch-inventory-magento-2#31.

@thomas-kl1
Copy link
Contributor Author

@cammonro things would be less complicated if the changes I suggest were approved. It's a minimal change for a greater support overall

@cammonro
Copy link
Contributor

cammonro commented Jun 23, 2025

@thomas-kl1

@cammonro things would be less complicated if the changes I suggest were approved. It's a minimal change for a greater support overall

Thanks for your thoughts on this.

A couple of things:

  1. The patch we are proposing on the inventory module is immediately shippable to our customers whereas the core module has other constraints.
  2. We are still intending to incorporate changes in the core module so these fixes are not mutually exclusive.

I also have a concern with your suggested approach as it relies on \Magento\InventoryCatalog\Plugin\CatalogInventory\Model\ResourceModel\Stock\Status\AdaptAddStockDataToCollectionPlugin::aroundAddStockDataToCollection to enforce MSI and we are still retaining deprecated APIs.

While the change is elegant, I personally prefer that we be more explicit here - as code that appears to use legacy logic like the Magento_CatalogInventory module (but is secretly MSI-aware via plugins) is in IMO ambiguous, misleading even, and could lead to maintenance issues down the road.

@thomas-kl1
Copy link
Contributor Author

Your concern are legitimate and I share them. However we didn't see any major changes in the Magento core and we don't expect any soon. Actually many parts of the Magento code is based on this similar mechanic. I don't see Adobe doing this refactor as it's really big. That's why I ended up doing this, would work for both cases (MSI or not) even if it use a ghost plugin (intended by MSI package to add coverage support on vanilla features)

@cammonro
Copy link
Contributor

Fair points!

Meanwhile, accurate stock quantities by warehouse were also a concern which the referenced PR aims to address.

Please note although we don't have a timeline we do intend to revisit this in the main module and your feedback will be incorporated. Marking this on hold for now. Thanks again for your help! We really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants