-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Category product count does not include products assigned to the category itself #40295
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
base: 2.4-develop
Are you sure you want to change the base?
Category product count does not include products assigned to the category itself #40295
Conversation
|
Hi @mimou78. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
1 similar comment
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
|
@magento run all tests |
…lf-reference-40263
…lf-reference-40263
…lf-reference-40263
app/code/Magento/Catalog/Model/ResourceModel/Category/Collection.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Catalog/Test/Unit/Model/ResourceModel/Category/CollectionTest.php
Show resolved
Hide resolved
|
@magento run all tests |
|
@magento run all tests |
…lf-reference-40263
|
@magento run Static Tests |
| $items = []; | ||
| $categoryIds = []; | ||
| for ($i = 1; $i <= $categoryCount; $i++) { | ||
| $category = $this->getMockBuilder(Category::class) |
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 we keep it as it is?
The implementation relies on the DataObject magic method and we explicitly add this method via
addMethods(['getIsAnchor'])
I don't like the idea of introducing \Magento\Catalog\Test\Unit\Helper\CategoryTestHelper because we:
- Creating new layer with a class that will be used only in tests
- Futher dependency on DataObject's magic method would require us extending this class for Categories and introducing even more classes for other entities.
Le't keep it as simple as possible and leave how it was before.
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.
Thanks for the feedback!
I fully understand your concern about introducing an extra layer that would only be used in tests.
The reason I went with this approach is that addMethods() is deprecated, and relying on DataObject magic methods is something Magento is gradually moving away from.
I followed the structure already used in the core—for example, Magento/Catalog/Test/Unit/Helper/ProductTestHelper. I tried to stay aligned with the same pattern for categories so the test suite remains consistent and future-proof.
My intention wasn’t to add unnecessary classes, but simply to avoid deprecated behavior and ensure our tests remain stable over time.
I’m of course open to discussion if you think we should take a different direction.
swnsma
left a comment
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.
Thank you for your changes!
Could you have a look at my small note about unit tests?
Summary
This pull request fixes an issue where the product count displayed in the Admin Category Grid
incorrectly shows 0 products for categories that do not have any descendants.
The current logic only counts products assigned to descendant categories and does not
include products directly assigned to the category itself.
Problem
When building the temporary table of category → descendant relationships, the SQL
logic only inserts rows for descendants, not for the category itself.
As a result:
Related Issue
Fixes #40263
What This Fix Does
This change ensures that each category always includes a self-reference entry in
the temporary category/descendant table.
As a result:
Technical Changes
(category_id = X, descendant_id = X)for everyprocessed category ID.
and the category itself.
Testing Instructions
Backward Compatibility
Risks
Low. This affects only the temporary table generation used for counting, not
the category model or product relations.