-
Notifications
You must be signed in to change notification settings - Fork 9.4k
28584 fixed issue category query does not handle fragments #28710
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
28584 fixed issue category query does not handle fragments #28710
Conversation
Hi @munkhulzii. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. |
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.
This looks good, and is working as expected for inline fragments.
However, it does not work for named fragements
e.g.
{
categoryList(filters: {ids: {in: ["11"]}}) {
...Cat
}
}
fragment Cat on CategoryTree {
name
position
url_key
}
throws error:
Argument 2 passed to Magento\\CatalogGraphQl\\Model\\Resolver\\Products\\DataProvider\\CategoryTree::joinAttributesRecursively() must be an instance of GraphQL\\Language\\AST\\FieldNode, instance of GraphQL\\Language\\AST\\FragmentSpreadNode given, called in /Users/drenaud/valet-magento/partners/app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/CategoryTree.php on line 144
28584 fix typo 28584 fix typo 28584 implement fragment spread for category
9c3c349
to
4511c46
Compare
app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/CategoryTree.php
Outdated
Show resolved
Hide resolved
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 think it would be preferable to pass resolveInfo down via methods arguments rather than setting it as a class member
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.
Generally looks good, just a few minor comments. Please rerun builds, I think there will be some static failures to address.
app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/CategoryTree.php
Show resolved
Hide resolved
@magento run all tests |
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.
check failing static tests.
unit tests are failing for some other reason, I wouldn't worry about them at this point
@magento run all tests |
@magento run all tests |
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.
Check web api functional tests
app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/ProductSearch.php
Outdated
Show resolved
Hide resolved
@magento run all tests |
Hi @munkhulzii, thank you for your contribution! |
Fixed issue #28584
fixed category query with inline fragments
fixed total_count of products in category