-
Notifications
You must be signed in to change notification settings - Fork 203
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
EZP-31107: Built-in query types for site building #2938
Conversation
Suggestion for a description update: you could shorten the examples by limiting them to the parameters part. That way, they can be used as is in both content view and query field configurations: content: '@=content.id'
field: 'related'
sort:
# Custom sort clause
target: \App\Repository\Content\Query\CustomSortClause
# Sorting direction
direction: desc
# Additional args passed to sort clause constructor
data: ['foo', 'bar', 'baz'] |
use eZ\Publish\API\Repository\Values\Content\Query\Criterion\LogicalNot; | ||
use eZ\Publish\API\Repository\Values\Content\Query\Criterion\MatchNone; | ||
|
||
final class AncestorsQueryType extends AbstractLocationQueryType |
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.
Missing description.
use eZ\Publish\API\Repository\Values\Content\Query\Criterion\MatchNone; | ||
use eZ\Publish\API\Repository\Values\Content\Query\Criterion\ParentLocationId; | ||
|
||
final class ChildrenQueryType extends AbstractLocationQueryType |
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.
Missing description.
use Symfony\Component\OptionsResolver\Options; | ||
use Symfony\Component\OptionsResolver\OptionsResolver; | ||
|
||
final class GeoLocationQueryType extends AbstractQueryType |
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.
Missing description.
use Symfony\Component\OptionsResolver\Options; | ||
use Symfony\Component\OptionsResolver\OptionsResolver; | ||
|
||
final class RelatedToContentQueryType extends AbstractQueryType |
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.
Missing description.
use eZ\Publish\API\Repository\Values\Content\Query\Criterion; | ||
use eZ\Publish\API\Repository\Values\Content\Query\Criterion\MatchNone; | ||
|
||
final class SiblingsQueryType extends AbstractLocationQueryType |
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.
Missing description.
Note: in the examples, you could replace |
45df5ca
to
f636bc9
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.
Nitpick: it should be BuiltIn
, with a t
, not with a d
.
A few suggestions for extensibility, but overall, it looks cool !
eZ/Bundle/EzPublishCoreBundle/DependencyInjection/EzPublishCoreExtension.php
Outdated
Show resolved
Hide resolved
|
||
eZ\Publish\Core\QueryType\BuildIn\AncestorsQueryType: | ||
tags: | ||
- { name: ezpublish.query_type, alias: 'eZ:Ancestors' } |
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 could get rid of most of that file with auto-configuration + a compiler pass:
- map
eZ\Publish\Core\QueryType\BuildIn\*
to theeZ:
query type namespace - use the file's name minus the
QueryType
prefix
By making the namespace mapping configurable and re-usable, we could even standardize automatic query type registration by naming convention. By default, the App\QueryType\*
namespace could be mapped to
App:
.
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.
- map
eZ\Publish\Core\QueryType\BuildIn\*
to theeZ:
query type namespace- use the file's name minus the
QueryType
prefixBy making the namespace mapping configurable and re-usable, we could even standardize automatic query type registration by naming convention. By default, the
App\QueryType\*
namespace could be mapped to
App:
.
Please don't do magic here. Do not rely either on namespace or class name. This is anti-pattern and a maintenance hell in the long run (future refactoring-wise).
Both namespace and class name are things that could change. We should not force anyone to use such convention.
Architecture-wise it should be a contract, which we already have.
I actually think you can auto-configure it already thanks to #2834, if memory serves.
eZ/Publish/Core/QueryType/BuildIn/SortSpec/SortClauseParser/MapDistanceSortClauseParser.php
Outdated
Show resolved
Hide resolved
Rebased after #2958 merge |
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 not a full review, but in general I don't like naming convention for Query Types.
eZ is a company-wide prefix for a group of Products. Query Types are specific to eZ Platform product and naming should reflect that.
For better readability I could agree on eZ:Platform:*
. This gives us future DXP expansion possibilities, e.g. eZ:Commerce:*
.
|
||
eZ\Publish\Core\QueryType\BuildIn\AncestorsQueryType: | ||
tags: | ||
- { name: ezpublish.query_type, alias: 'eZ:Ancestors' } |
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.
- map
eZ\Publish\Core\QueryType\BuildIn\*
to theeZ:
query type namespace- use the file's name minus the
QueryType
prefixBy making the namespace mapping configurable and re-usable, we could even standardize automatic query type registration by naming convention. By default, the
App\QueryType\*
namespace could be mapped to
App:
.
Please don't do magic here. Do not rely either on namespace or class name. This is anti-pattern and a maintenance hell in the long run (future refactoring-wise).
Both namespace and class name are things that could change. We should not force anyone to use such convention.
Architecture-wise it should be a contract, which we already have.
I actually think you can auto-configure it already thanks to #2834, if memory serves.
master
Implemented Query Types
Ancestors/Children/Siblings
Ancestors/Children/Siblings of content or location.
location
content
Subtree
Subtree of given location/content.
location
content
depth
-1
by default which means the whole subtreeGeoLocation
Geographic proximity (map location)
field
distance
latitude
longitude
operator
<=
RelatedToContent
Related content
content
field
Common parameters
Parameters accepted by all build-in Query Types.
filter
limit
offset
sort
Common filters
content_type
null
['article', 'blog_post']
visible_only
true
false
siteaccess_aware
true
false
Sorting
Expected results order could be specified using
sort
option. Accepted values are\eZ\Publish\API\Repository\Values\Content\Query\SortClause
/\eZ\Publish\API\Repository\Values\Content\Query\SortClause[]
or string which is sort specification. For example:How to provide support for custom sort clause?
In case of simple sort clauses (accepts only sort direction in construtor parameter) use
eZ\Publish\Core\QueryType\BuiltIn\SortSpec\SortClauseParser\DefaultSortClauseParser
:In more advanced cases implement
\eZ\Publish\Core\QueryType\BuiltIn\SortSpec\SortClauseParserInterface
TODO:
$ composer fix-cs
).