Doctrine filters: Allow nested properties (across relations) #242

teohhanhui · 2015-08-25T12:20:38Z

I'm targeting #83

theofidry · 2015-08-25T13:19:37Z

Doctrine/Orm/Filter/OrderFilter.php

+
+                foreach ($propertyParts['associations'] as $association) {
+                    $alias = sprintf('%s_%s', self::DQL_NAMESPACE, $association);
+                    $queryBuilder->join(sprintf('%s.%s', $parentAlias, $association), $alias);


So if you got several properties with the same parent you will end up doing the join several times right?

If you mean two or more nested properties through the same relation, then yes. But I'm not sure if it'll cause any real issues (will need to test that).

If I remember right it throws an error.

theofidry · 2015-08-25T13:23:51Z

Regarding the tests it might be worth to have fixtures specific to features/scenarios. This can be easily achieved with:

You can find an example here.

teohhanhui · 2015-08-27T12:38:25Z

@theofidry Would you like to help write some new tests?

@dunglas @sroze This is ready for review.

teohhanhui · 2015-08-27T12:40:04Z

Doctrine/Orm/Extension/PaginationExtension.php

+     *
+     * @return bool
+     */
+    private function useOutputWalkers(QueryBuilder $queryBuilder)


Ideally this should belong in Doctrine ORM?

At least in an other place than here yep :)

What is the problem to have it here?

I just thought it could be useful in Doctrine ORM (of course, that's subject to the opinion of Doctrine maintainers).

@dunglas I too don't get why this should be in another place, when it's specific to Doctrine pagination. But then again I'm not sure I can understand very well the other strong opinions of @sroze

theofidry · 2015-08-27T12:42:49Z

I'll try to see but I'm very busing moving right now :(

I'm testing the two libraries for such tests for the first time with theofidry/LoopBackApiBundle#17 and as there is some things in common I think I may help. You're doing something I wished to do for quite some time now so that the least I can do. I'll take a good look tomorrow but can't guarantee anything.

sroze · 2015-08-31T12:02:34Z

Doctrine/Orm/Extension/PaginationExtension.php

     */
-    public function __construct(RequestStack $requestStack)
-    {
+    public function __construct(


You can keep the parameters on the same line

teohhanhui · 2015-09-01T11:22:14Z

Doctrine/Orm/Util/QueryUtils.php

+     *
+     * @return string
+     */
+    public static function getJoinRelationship(Join $join)


@sroze #242 (diff) has been moved here but remains unresolved.

dunglas · 2015-10-04T17:33:54Z

@vincentchalamon it's the PR I was talking about.

vincentchalamon · 2015-10-05T13:16:07Z

Doctrine/Orm/Filter/AbstractFilter.php

+     *
+     * @return ClassMetadata
+     */
+    protected function getNestedMetadata(ResourceInterface $resource, $associations)


protected function getNestedMetadata(ResourceInterface $resource, array $associations)

theofidry · 2015-10-22T16:56:15Z

@dunglas to be fair it will require a good amount of work, besides I didn't finished on my end and is not really available either so if we wish to wait it's gonna take another couple of weeks (I'm not saying we should merge this now, but just want to make sure you're taking that into account)

teohhanhui · 2015-10-22T18:48:40Z

@dunglas This is in master.

sroze · 2015-10-22T19:54:09Z

Util/RequestUtils.php

+     *
+     * @return array
+     */
+    public static function parseRequestParams($source)


This method is never used outside of that class so can be private.

sroze · 2015-10-23T07:55:23Z

I'm okay to merge the PR without the optimisation of the filters, but not with these 2 classes RequestUtils and QueryUtils. Please find a better name for them, that's all I want, even I think that if we are introducing this kind of classes means there's something wrong in our design (might be from before your PR).

teohhanhui · 2015-10-23T08:02:25Z

@sroze What do you have in mind?

theofidry · 2015-10-23T08:19:39Z

I think that if we are introducing this kind of classes means there's something wrong in our design (might be from before your PR).

Too me it just looks like the strategy to extract values from the request and the logic to construct our queries have become more complex and should now be dedicated classes. It made sense like it was before as there is nothing wrong with trying to keep things simple, but as the complexity grow a lot with this filter, it calls for some change.

dunglas · 2015-10-27T06:27:23Z

features/doctrine/date-filter.feature

@@ -10,7 +10,7 @@ Feature: Date filter on collections
    Then the response status code should be 200
    And the response should be in JSON
    And the header "Content-Type" should be equal to "application/ld+json"
-    And the JSON should be equal to:
+    And the JSON should be valid according to this schema:


The idea of using a JSON Schema instead of raw values is great but I what is the interest here has the schema isn't provided?

I prefer using this method cause when I update configuration, I need to update every feature file about schema. There is no need on it as scenario just need to check configuration. Full schema is already tested on features/crud.feature

vincentchalamon · 2015-11-04T13:54:33Z

Fix in teohhanhui#4

bwegrzyn · 2015-11-11T14:53:08Z

Just wanted to leave a +1 to show interest in this change. We currently have to issue 3 API requests for something that really should be doable with 1. Can't wait to try out these changes when they are merged.

- Fix from GitHub comments - Rename Request & Query factory class - Fix request get content

teohhanhui · 2015-11-12T07:02:13Z

Commits from @vincentchalamon merged and squashed.

Doctrine filters: Allow nested properties (across relations)

dunglas · 2015-11-12T07:05:45Z

Thank you very much for this hard work @teohhanhui and @vincentchalamon

vincentchalamon · 2015-11-12T08:29:41Z

Thanks @dunglas. Can you release it soon ?

theofidry reviewed Aug 25, 2015
View reviewed changes

teohhanhui force-pushed the filter_nested branch 6 times, most recently from 1ad2d44 to d4eeca5 Compare August 27, 2015 12:35

teohhanhui changed the title ~~[WIP] Doctrine filters: Allow nested properties (across relations)~~ Doctrine filters: Allow nested properties (across relations) Aug 27, 2015

teohhanhui reviewed Aug 27, 2015
View reviewed changes

sroze reviewed Aug 31, 2015
View reviewed changes

sroze mentioned this pull request Aug 31, 2015

Added Doctrine helper #195

Closed

teohhanhui force-pushed the filter_nested branch 4 times, most recently from 6200e1d to 9ef0887 Compare September 1, 2015 10:35

teohhanhui reviewed Sep 1, 2015
View reviewed changes

vincentchalamon reviewed Oct 5, 2015
View reviewed changes

sroze reviewed Oct 22, 2015
View reviewed changes

dunglas mentioned this pull request Oct 26, 2015

Allow abstract resource #311

Merged

2 tasks

dunglas reviewed Oct 27, 2015
View reviewed changes

vincentchalamon mentioned this pull request Nov 4, 2015

Fix filters review teohhanhui/api-platform-core#3

Closed

2 tasks

teohhanhui force-pushed the filter_nested branch from 4650775 to 2cc6f44 Compare November 5, 2015 08:03

vincentchalamon mentioned this pull request Nov 5, 2015

Fix from GitHub comments teohhanhui/api-platform-core#4

Merged

1 task

teohhanhui and others added 2 commits November 12, 2015 14:58

Doctrine filters: Allow nested properties (across relations)

f5f2b79

Add tests

4481c6d

- Fix from GitHub comments - Rename Request & Query factory class - Fix request get content

teohhanhui force-pushed the filter_nested branch from 0b38817 to 4481c6d Compare November 12, 2015 07:01

dunglas added a commit that referenced this pull request Nov 12, 2015

Merge pull request #242 from teohhanhui/filter_nested

1101920

Doctrine filters: Allow nested properties (across relations)

dunglas merged commit 1101920 into api-platform:master Nov 12, 2015

teohhanhui deleted the filter_nested branch November 12, 2015 07:08

vincentchalamon mentioned this pull request Nov 19, 2015

Doctrine filters: Allow nested properties (across relations) #337

Merged

teohhanhui mentioned this pull request Nov 21, 2015

Add documentation for filter on nested properties api-platform/docs#24

Merged

blaues0cke mentioned this pull request May 9, 2017

Filter not working with embedded properties (multiple levels) api-platform/api-platform#297

Closed

teohhanhui mentioned this pull request Dec 19, 2018

Add ExistsFilter #906

Merged

lukasluecke mentioned this pull request Jul 13, 2020

[3.0] Issues with nested properties (dot notation) in fields EasyCorp/EasyAdminBundle#3389

Closed

Uh oh!

Doctrine filters: Allow nested properties (across relations) #242

Doctrine filters: Allow nested properties (across relations) #242

Uh oh!

Conversation

teohhanhui commented Aug 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theofidry commented Aug 25, 2015

Uh oh!

teohhanhui commented Aug 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theofidry commented Aug 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Oct 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theofidry commented Oct 22, 2015

Uh oh!

teohhanhui commented Oct 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze commented Oct 23, 2015

Uh oh!

teohhanhui commented Oct 23, 2015

Uh oh!

theofidry commented Oct 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentchalamon commented Nov 4, 2015

Uh oh!

bwegrzyn commented Nov 11, 2015

Uh oh!

teohhanhui commented Nov 12, 2015

Uh oh!

dunglas commented Nov 12, 2015

Uh oh!

vincentchalamon commented Nov 12, 2015

Uh oh!

Uh oh!