-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Make product list filter counts dependent on product visibility configuration #6035
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
Make product list filter counts dependent on product visibility configuration #6035
Conversation
…_array check that fails in case integer and string values are compared
…r the return value
… even when only one product re-indexing was ordered
@@ -265,7 +265,7 @@ public function addFieldToFilter($field, $condition = null) | |||
|
|||
$this->getSearchCriteriaBuilder(); | |||
$this->getFilterBuilder(); | |||
if (!is_array($condition) || !in_array(key($condition), ['from', 'to'])) { | |||
if (!is_array($condition) || !in_array((string)key($condition), ['from', 'to'])) { |
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.
we can use strict flag: like in_array(key($condition), ['from', 'to'], true)
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.
true that. was not aware of the 3rd argument :)
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.
I can confirm that the suggested change by @v-r along with this pull request do solve the problem. We need this fix which we've made on our local install. @allanpaiste any chance you can update this one line and submit the PR again? I could make a new request but I'd just be copying your work!
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.
changed & pushed. I don't mind if my work gets copied as long as Magento gets better :)
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.
Okay it seems that the build fails with pretty cryptic error. Considerign that develop branch itself is all in red, I would rather wait the PR target to go to be back in green before looking into that one.
Considering the narrow scope of the change I really doubt that it's caused by this branch.
@allanpaiste please look into issues with your PR |
@allanpaiste i used 2.1.2,when i replace AbstractEav.php from https://github.com/magento/magento2/pull/6035/files and flush cache, and reindex..
how to fix it? |
Hi @allanpaiste How does current fix could address issue #5670, if Layered Navigation uses CatalogSearch Fulltext index and there are dedicated filters for visibility there specified in search_request.xml file |
*/ | ||
protected function _prepareVisibilityIndex($entityIds = null) | ||
{ | ||
$attribute = $this->_eavConfig->getAttribute(\Magento\Catalog\Model\Product::ENTITY, 'visibility'); |
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.
It's better to use constant from Public Service Contract API
\Magento\Catalog\Api\Data\ProductAttributeInterface::ENTITY_TYPE_CODE
protected function _prepareVisibilityIndex($entityIds = null) | ||
{ | ||
$attribute = $this->_eavConfig->getAttribute(\Magento\Catalog\Model\Product::ENTITY, 'visibility'); | ||
$this->_prepareIndex($entityIds, $attribute->getAttributeId()); |
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 have additional call for _prepareIndex? if we had one before in chaining call.
This code looks awkward for me, as we have:
$this->_prepareIndex()->_prepareRelationIndex()->_prepareIndex($entityIds, $attribute->getAttributeId())->_removeNotVisibleEntityFromIndex();
The fact that we should to call the same method twice says about code smell
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.
The additional call to _prepareIndex is necessary for this to work. Removing it results in the same incorrect visibility behavior.
Fixes the issue that was caused by two factors:
Honestly tried to add a tests to this change as well, but instantiating product collection is a MAJOR head-ache due to things getting created, accessed and type-checked left and right - so - after seeing that just the setup for a test was going to be 100+ lines long - decided against creating it as it would just be unmaintainable.
Addresses issue: #5670