Skip to content
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

Closed
wants to merge 19 commits into from

Conversation

larsroettig
Copy link
Member

No description provided.

- Remove Mview Entry will not work with MSI correctly
- Impelment InventoryCatalogSearch Module
- Add new Extensions Point for CatalogSearch Stock
@larsroettig larsroettig self-assigned this Dec 14, 2017
@larsroettig larsroettig changed the title WIP MSI: #302 (Refactoring CatalogSearch Module) MSI: #302 (Refactoring CatalogSearch Module) Dec 14, 2017
@vadimjustus vadimjustus self-assigned this Dec 19, 2017
@smoskaluk
Copy link

@larsroettig I have created simple product with qty on default source - but it is not displaying on the homepage.

Copy link
Contributor

@maghamed maghamed left a 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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);
Copy link
Contributor

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';

Copy link
Contributor

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()],
Copy link
Contributor

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';
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be optional

-  Fix static tests
-  Fix intigrations tests
@naydav
Copy link

naydav commented Jan 10, 2018

This PR is closed due to this PR is not ready for merge:

  1. We have broken functionality in this PR (we have fatal errors on frontend)
  2. We have a lot of failed integration tests
  3. We have a lot of redundant code in this PR like StockJoinProvider
    I will create a few tasks for finishing this functionality in proper way

@naydav naydav closed this Jan 10, 2018
@maghamed maghamed deleted the task/302-catalog-search branch December 11, 2018 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants