-
Notifications
You must be signed in to change notification settings - Fork 248
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
MSI: #302 (Refactoring CatalogSearch Module) #304
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing extension points are not clear and self-descriptive.
Looks like M1 programming, working with low-level Select object and applying different joins/filters
*/ | ||
public function __construct( | ||
Config $eavConfig, | ||
ResourceConnection $resource, | ||
ScopeResolverInterface $scopeResolver, | ||
Session $customerSession | ||
Session $customerSession, | ||
StockSelectProviderInterface $selectProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new parameter must me optional
interface StockSelectProviderInterface | ||
{ | ||
/** | ||
* Returns the stock select by current scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
*/ | ||
public function __construct( | ||
AliasResolver $aliasResolver, | ||
CustomAttributeFilter $customAttributeFilter, | ||
FilterStrategyInterface $filterStrategy, | ||
VisibilityFilter $visibilityFilter, | ||
StockStatusFilter $stockStatusFilter | ||
StockStatusFilterInterface $stockStatusFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's sounds like violation of modularity principles
@@ -58,13 +64,15 @@ public function __construct( | |||
ResourceConnection $resourceConnection, | |||
EavConfig $eavConfig, | |||
ScopeConfigInterface $scopeConfig, | |||
AliasResolver $aliasResolver | |||
AliasResolver $aliasResolver, | |||
StockJoinProviderInterface $joinProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be optional
This PR is closed due to this PR is not ready for merge:
|
No description provided.