-
Notifications
You must be signed in to change notification settings - Fork 70
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
FEATURE: Add Node-Type Filter #398
base: master
Are you sure you want to change the base?
Conversation
Simplified example: public function getVehiclePages(NodeInterface $siteNode)
{
$filter = [
'Foo.Bar:Document.Vehicle',
'Foo.Bar:Document.Motorcyle',
'!Neos.Neos:Document' // this nodetype will be excluded
];
$queryBuilder = new ElasticSearchQueryBuilder();
$queryBuilder->query($siteNode);
$queryBuilder->nodeTypeFilter(implode(',', $filter));
// [...]
} |
e9b1557
to
c750f23
Compare
@Sebobo @daniellienert can you review this PR? |
c750f23
to
b699e46
Compare
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.
Hey @erkenes, thanks a lot for the improvement!
I have two comments tho.
Cheers Daniel
*/ | ||
public function nodeTypeFilter(array $expectedNodeTypes, array $excludedNodeTypes): QueryBuilderInterface | ||
{ | ||
$excludeShould = []; |
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.
Nitpick: It ends in an bool must / must_not query so should is a bit missleading here
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 changed Should
to Terms
* @throws QueryBuildingException | ||
* @api | ||
*/ | ||
public function nodeTypeFilter(array $expectedNodeTypes, array $excludedNodeTypes): QueryBuilderInterface |
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.
Would be nice if you would add a slim test in ElasticSearchQueryTest
.
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 added a test and adjusted the existing tests to match the new circumstances.
- Creates a method which allows to create a nodetype filter to include and exclude specific nodetypes - Adds a test for the new filter function - Adjusts existing tests to match the new circumstances
b699e46
to
2b66844
Compare
Create a method which allows to create a nodetype filter to include and exclude specific nodetypes