MSI: #302 (Refactoring CatalogSearch Module)#304
MSI: #302 (Refactoring CatalogSearch Module)#304larsroettig wants to merge 19 commits intodevelopfrom
Conversation
- Remove Mview Entry will not work with MSI correctly - Impelment InventoryCatalogSearch Module - Add new Extensions Point for CatalogSearch Stock
…si into task/302-catalog-search
…ation for StockSelectProvider
|
@larsroettig I have created simple product with qty on default source - but it is not displaying on the homepage. |
maghamed
left a comment
There was a problem hiding this comment.
Existing extension points are not clear and self-descriptive.
Looks like M1 programming, working with low-level Select object and applying different joins/filters
| ScopeResolverInterface $scopeResolver, | ||
| Session $customerSession | ||
| Session $customerSession, | ||
| StockSelectProviderInterface $selectProvider |
There was a problem hiding this comment.
new parameter must me optional
| interface StockSelectProviderInterface | ||
| { | ||
| /** | ||
| * Returns the stock select by current scope |
There was a problem hiding this comment.
what is stock select?
why do we need it?
Please describe that in comments. Also select is not good extension point, because you don't know fields and tables in select you can reffecrence to
| FilterStrategyInterface $filterStrategy, | ||
| VisibilityFilter $visibilityFilter, | ||
| StockStatusFilter $stockStatusFilter | ||
| StockStatusFilterInterface $stockStatusFilter |
There was a problem hiding this comment.
all the changes to existing code of catalog search - supposed to be backward compatible
| use Magento\Framework\DB\Select; | ||
|
|
||
| /** | ||
| * SPI Interface to change the stock join |
There was a problem hiding this comment.
description of the interface looks awkward.
It's not clear the main responsibility of this entity and why we need it
| * @param $alias | ||
| * @return void | ||
| */ | ||
| public function add(Select $select, $alias); |
There was a problem hiding this comment.
why do we need the interface which adds JOIN to some Select object?
Why it's not enough just to introduce table resolver, which returns stock index table based on context ?
| * (e.g. for configurable products and options) | ||
| */ | ||
| const FILTER_ENTITY_AND_SUB_PRODUCTS = 'filter_with_sub_products'; | ||
|
|
There was a problem hiding this comment.
these are backward incompatible changes
| private function addInventoryStockJoin(Select $select, $showOutOfStockFlag) | ||
| { | ||
| $select->joinInner( | ||
| [static::TABLE_ALIAS_STOCK_INDEX => $this->getStockTableName()], |
There was a problem hiding this comment.
why do you use static for access to all constants?
are you expecting to use late static binding here?
but constants could not be overloaded
| */ | ||
| const FILTER_JUST_ENTITY = 'general_filter'; | ||
| const FILTER_ENTITY_AND_SUB_PRODUCTS = 'filter_with_sub_products'; | ||
| const FILTER_JUST_SUB_PRODUCTS = 'filter_just_sub_products'; |
There was a problem hiding this comment.
that's sounds like violation of modularity principles
| ScopeConfigInterface $scopeConfig, | ||
| AliasResolver $aliasResolver | ||
| AliasResolver $aliasResolver, | ||
| StockJoinProviderInterface $joinProvider |
|
This PR is closed due to this PR is not ready for merge:
|
No description provided.