EZP-26273: Use QueryType for the top menu #39
Conversation
clash82
force-pushed
the
query-controller-for-menu
branch
3 times, most recently
from
ab9b2a3 to
92940b0
Compare
clash82
changed the title
|
+1 |
1 similar comment
|
+1 |
| - '@ezpublish.api.service.location' | ||
| - '@ezpublish.config.resolver' | ||
| - '@app.criteria.menu' | ||
| - '@ezpublish.query_type.registry' |
There was a problem hiding this comment.
You could simplify dependencies here by passing the QueryType directly: - '@app.query_type.menu'. Then you don't need to use the registry anymore.
There was a problem hiding this comment.
Yes, I know. It's only for demonstration purpose, look: https://github.com/ezsystems/ezplatform-demo/pull/39/files#diff-71435085ac34f5c4748063a144167686R43
There was a problem hiding this comment.
Hmm, I'm not sure it's really relevant to demonstrate the complicated way, and comment out with the simplest one. I'd instead inject the query type, and explain that the registry could be used instead.
Both work, of course.
There was a problem hiding this comment.
Changed. If it's too complicated we should be simplify it :)
clash82
force-pushed
the
query-controller-for-menu
branch
7 times, most recently
from
11e99ad to
7419c94
Compare
|
@bdunogier can I ask you for a final review? |
|
big boss, can you have a look? ;) @bdunogier |
| /** | ||
| * @param string[] $value | ||
| */ | ||
| public function setLanguages($value) |
There was a problem hiding this comment.
If the parameter is always an array, you should typehint it as such.
There was a problem hiding this comment.
Good point! I forgot about it.
| new Query\Criterion\Visibility(Query\Criterion\Visibility::VISIBLE), | ||
| new Query\Criterion\Location\Depth( | ||
| Query\Criterion\Operator::BETWEEN, | ||
| [ |
There was a problem hiding this comment.
No need to wrap this array's content on multiple lines, it will be as readable on one line. Importing eZ\Publish\API\Repository\Values\Content\Query\Criterion in the current namespace would also allow you to shorten this down:
$criteria = new Criterion\LogicalAnd([
new Criterion\Visibility(Criterion\Visibility::VISIBLE),
new Criterion\Location\Depth(Criterion\Operator::BETWEEN, [1, 2]),
new Criterion\Subtree('/1/2/'),
new Criterion\LanguageCode($this->languages),
Also, could the depth be handled as new Criterion\Location\Depth(Criterion\Operator::LTE, 2) ?
There was a problem hiding this comment.
Works like a charm, thank you, didn't know that!
clash82
force-pushed
the
query-controller-for-menu
branch
from
7419c94 to
76339c8
Compare
it just stops working one day ;) |
https://jira.ez.no/browse/EZP-26273