-
Notifications
You must be signed in to change notification settings - Fork 201
Feature: Query content view controller #1675
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: Query content view controller #1675
Conversation
|
hi @bdunogier , a question about Edit: now i see in the md you're quoting it, so looks it will work |
|
It should have been quoted, I'll fix the example. So the answer is "yes" :-) |
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.
nitipick: Location
|
Looks good to me. Don't forget to add some phpdoc before 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.
This doesn't actually say that the children of the given location id are being returned as results. Do you consider that irrelevant to the feature, since it's up to the custom QueryType?
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 is a fair question. We could add a sentence that asserts that the results are those we expect. It is kind of what "the Query results" means here, but it needs to be made more explicit. "The results of the LocationChidren query" are assigned to the children twig variable ?
|
Seems good. I second Yannick on doc. |
|
How about pagination support to return pager instance? |
|
Pushed an improved version of the configuration. Let's use the behat scenario to discuss: https://github.com/ezsystems/ezpublish-kernel/pull/1675/files#diff-6d2810af82a8d9056f00de53f69e3702 :-) |
|
First behat implementation added, may pass. Will need refactoring :) |
34121c8 to
4ab96fd
Compare
|
BDD implemented and passing ! |
|
@wizhippo as much as I'd like to do it, I'm not sure it can fit before the scope freeze (tomorrow). How would you see that working when it comes to assigning pagination data to the template (naming, mainly) ? |
4ab96fd to
c1bd329
Compare
| @@ -0,0 +1,58 @@ | |||
| <?php | |||
| /** | |||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | |||
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 guess we haven't fully (re)defined this, but I would have thought heading should be:
<?php
/**
* File containing the BlaBla class.
*
* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/Ideally skipping the file description (File containing the BlaBla class) in favor of rather spending the effort by having more useful class description, including usage examples.
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'd go for @license and @copyright alone, I don't really see the point of the first line anyway, and it seems that we don't really care about it.
Agreed this is a follow up topic. Very Side comment on that, no need to discuss this here, more for when we tackle this perhaps: If we want to think a bit forward we should maybe rather think of using items as offsets, like github does, and as recommended for misc reasons, but will be difficult to handle sorting in a good way for the API so might not be applicable. |
| /** | ||
| * @param ContentView $view | ||
| */ | ||
| public function runQuery(ContentView $view, $method) |
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.
private
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.
fixed
2d5a478 to
7e325f4
Compare
|
+1 🐴 besides license nitpick. You have quite a bit to demo tomorrow ;) |
QueryTypeContentViewMapper is now an interface, in order to document both the input and output expectations. The existing implementation is renamed to QueryParameterContentViewQueryTypeMapper.
43f0caf to
2acb2e1
Compare
|
Files headers taken care of. |
|
+1 |
|
And here we go. |
In this approach, a built-in
QueryControlleris added. It can be used in anycontent_viewconfig to add the results of aQueryTypeto the view. See the BDD specs linked above for code examples.The view is configured to use
ez_query:contentQueryAction. The controller action runs the QueryType specified asquery, and sets the results to the variable specified asvariable.The elements from
queryParametersare passed to the QueryType'sgetQuery()method.The expression language can be used in any queryParameter by prefixing the value with
@=. The expression is given theview, thelocationand thecontent. This makes it easy to pass any property from those value objects to generate the query.A working configuration example, with a QueryType, can be found in the BDD scenario.