[RFR] Issues/1246 #1324

vincentchalamon · 2017-08-22T13:01:23Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1246, #1323 (comment)
License	MIT
Doc PR	api-platform/docs#269

dunglas · 2017-08-22T13:04:34Z

src/Bridge/Doctrine/Orm/Extension/OrderExtension.php

                        $field = $order;
                        $order = 'ASC';
                    }
-                    $queryBuilder->addOrderBy('o.'.$field, $order);
+                    if (false === strstr($field, '.')) {


Can't you use strpos and store the result in a variable to use it if it doesn't return false? It will remove one function call.

meyerbaptiste · 2017-08-22T15:06:41Z

src/Bridge/Doctrine/Orm/Extension/OrderExtension.php

+                        // Ensure query join is done on association
+                        $associationName = substr($field, 0, $pos);
+                        if (!in_array($associationName, $queryBuilder->getRootAliases(), true)) {
+                            $queryBuilder->join('o.'.$associationName, $associationName);


No conflicts with src/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtension.php#L117-L124 here (extensions priorities)?

@dunglas WDYT?

Can you use the JoinOnce thing? This may lead to multiple joins indeed.

@soyuka What do you mean? Do you have any example please?

Maybe this:

core/src/Bridge/Doctrine/Orm/Filter/AbstractFilter.php

Lines 382 to 401 in 0e1af77

protected function addJoinOnce(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $association): string

{

$join = $this->getExistingJoin($queryBuilder, $alias, $association);

if (null === $join) {

$associationAlias = $queryNameGenerator->generateJoinAlias($association);

if (true === QueryChecker::hasLeftJoin($queryBuilder)) {

$queryBuilder

->leftJoin(sprintf('%s.%s', $alias, $association), $associationAlias);

} else {

$queryBuilder

->innerJoin(sprintf('%s.%s', $alias, $association), $associationAlias);

}

} else {

$associationAlias = $join->getAlias();

}

return $associationAlias;

}

What do you think about a QueryBuilderHelper, which can be shared with filters & extensions?

Would be nice yeah, maybe we can do this in another PR!

I better think it could be done in this PR as its totally related to it. I could create this QueryBuilderHelper and move addJoinsForNestedProperty, getExistingJoin & addJoinOnce methods in it.

But I'm not sure it's the best approach, I think it should be better to call these methods on the QueryBuilder object directly instead of using an external service which modify the query builder object. Something like $queryBuilder->addJoinOnce instead of QueryBuilderHelper::addJoinOnce($queryBuilder), wdyt @soyuka?

The helper (as long as it is marked @internal) looks good to me. The QueryBuilder is provided by Doctrine, it's not really wanted to modify it.

I'd keep static helpers instead of extending the QueryBuilder.

dunglas · 2017-09-18T15:12:33Z

src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php

+     *
+     * @return string the new association alias
+     */
+    public static function addJoinOnce(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $association, ?string $joinType = null, ?string $conditionType = null, ?string $condition = null): string


You can't use the ?string syntax because we ensure compatibility with PHP 7.0 (this syntax was introduced in 7.1).

soyuka · 2017-09-18T15:34:50Z

src/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtension.php

+            /** @var Join $joinPart */
+            $joinString = str_replace($aliases, $replacements, $joinPart->getJoin());
+            $alias = substr($joinString, 0, strpos($joinString, '.'));
+            $association = substr($joinString, strpos($joinString, '.')+1);


can we avoid multiple calls on strpos?

Do you think it's possible that strpos returns false here? Maybe we kinda need a test?

@soyuka As it's from a cloned query builder, I don't think it's even possible that strpos could return false. It would mean that the original query builder is invalid, which report the bug to doctrine.

soyuka · 2017-09-18T15:37:52Z

tests/Fixtures/TestBundle/Entity/Foo.php

@@ -42,15 +41,13 @@ class Foo
     * @var string The foo name
     *
     * @ORM\Column
-     * @Assert\NotBlank


Really not needed? I'd have think those are for a given behat test, if not np just remove them though it's not part of the fix (or is it?).

Originally, I created this Foo entity for my tests. It was copied/pasted from another one. But I think the assertion is useless on this entity as I only create the objects by myself without validation.
This modification (in this PR) is a clean :-)

meyerbaptiste

It would be nice to compare SQL queries before and after these changes to verify that they are identical. These ones seem to me very sensitive...

meyerbaptiste · 2017-10-02T12:54:46Z

src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php

+ *
+ * @internal
+ */
+final class QueryBuilderHelper


Can you add a private constructor?

Because he likes private constructors :P.

Nah just that it's a convention when classes have no instances (ie are never called with new because they only have statics).

vincentchalamon · 2017-10-16T08:31:22Z

It would be nice to compare SQL queries before and after these changes to verify that they are identical. These ones seem to me very sensitive...

@meyerbaptiste on which class: FilterEagerLoadingExtension or QueryBuilderHelper?

soyuka · 2017-10-16T10:09:11Z

@meyerbaptiste He didn't changed the tests and we do test queries there.

soyuka · 2017-10-16T10:10:13Z

src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php

+    {
+        $parts = $queryBuilder->getDQLPart('join');
+
+        if (!isset($parts['o'])) {


Maybe we could put this o alias in some constant somewhere (in another PR).

It could be easily retried by $queryBuilder->getRootAliases()[0]. Should I replace it here?

don't IMO it's fine like this. It's just that we have a lot of these 'o' alias hard-coded in the bridge.

soyuka · 2017-11-06T14:37:36Z

Would you mind fixing phpstan?

vincentchalamon · 2017-11-06T14:51:50Z

@soyuka I don't understand where the error comes from in my code:

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Error                                                                                                                                                                                                                    
 ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Ignored error pattern #Parameter \#2 \$dqlPart of method Doctrine\\ORM\\QueryBuilder::add\(\) expects Doctrine\\ORM\\Query\\Expr\\Base, Doctrine\\ORM\\Query\\Expr\\Join\[\] given# was not matched in reported errors.  
 ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
                                                                                
 [ERROR] Found 1 error                                                          
                                                                                
The command "if [[ $lint = 1 ]]; then phpstan analyse -c phpstan.neon -l5 --ansi src tests; fi" exited with 1.

soyuka · 2017-11-06T15:20:42Z

Just remove this line:
https://github.com/api-platform/core/blob/master/phpstan.neon#L14

soyuka · 2017-11-06T15:49:53Z

tests/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtensionTest.php

-        $queryNameGenerator->generateJoinAlias('item')->shouldBeCalled()->willReturn('item_2');
-        $queryNameGenerator->generateJoinAlias('label')->shouldBeCalled()->willReturn('label_2');
+        $queryNameGenerator->generateJoinAlias('compositeItem')->shouldBeCalled()->willReturn('item_2');
+        $queryNameGenerator->generateJoinAlias('compositeLabel')->shouldBeCalled()->willReturn('label_2');


This change is wrong, item and label are correct I think (at least according to failing tests).

Nop, cause this call is made through QueryBuilderHelper::addJoinOnce from association name.

I don't understand why travis is failing…on a non-existing unit test 😮 (failing method testCompositeIdentifiersWithoutAssociation does not exist in FilterEagerLoadingExtensionTest)

I'll try to squash my changes to force travis to rebuild its environment

I added this recently maybe a bad rebase?
https://github.com/api-platform/core/blob/d18ec0da1ac52f92ca755ca275cd0853d0972878/tests/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtensionTest.php

Rebase missing, I fixed it, thanks @soyuka

soyuka · 2017-11-07T08:57:04Z

👍 waiting for some more review but this LGTM!

dunglas

Some minor changes. 👍 otherwise

dunglas · 2017-11-09T11:38:56Z

src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php

+    /**
+     * Adds a join to the queryBuilder if none exists.
+     *
+     * @param QueryBuilder                $queryBuilder


You can remove most of these @param.

dunglas · 2017-11-09T11:41:05Z

src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php

+     */
+    public static function addJoinOnce(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $association, $joinType = null, $conditionType = null, $condition = null): string
+    {
+        $join = self::getExistingJoin($queryBuilder, $alias, $association);


What about:

if (null !== $join) { return $join->getAlias(); } $associationAlias = $queryNameGenerator->generateJoinAlias($association); $query = "$alias.$association"; if (Join::LEFT_JOIN === $joinType || QueryChecker::hasLeftJoin($queryBuilder)) { $queryBuilder->leftJoin($query, $associationAlias, $conditionType, $condition); } else { $queryBuilder->innerJoin($query, $associationAlias, $conditionType, $condition); } return $associationAlias;

To reduce the complexity of this method.

dunglas · 2017-11-09T11:41:40Z

src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php

+    /**
+     * Get the existing join from queryBuilder DQL parts.
+     *
+     * @param QueryBuilder $queryBuilder


All @param can be removed

dunglas · 2017-11-09T13:18:09Z

Thanks @vincentchalamon!

vincentchalamon self-assigned this Aug 22, 2017

vincentchalamon requested review from dunglas, Simperfit and meyerbaptiste August 22, 2017 13:01

dunglas requested changes Aug 22, 2017

View reviewed changes

dunglas approved these changes Aug 22, 2017

View reviewed changes

meyerbaptiste reviewed Aug 22, 2017

View reviewed changes

vincentchalamon changed the title ~~Issues/1246~~ [WIP] Issues/1246 Aug 30, 2017

meyerbaptiste mentioned this pull request Sep 11, 2017

Fix api-platform/core #1246: configure default order on ApiResource annotation api-platform/docs#235

Merged

vincentchalamon changed the title ~~[WIP] Issues/1246~~ [RFR] Issues/1246 Sep 18, 2017

dunglas requested changes Sep 18, 2017

View reviewed changes

soyuka reviewed Sep 18, 2017

View reviewed changes

vincentchalamon changed the base branch from master to 2.1 September 21, 2017 14:06

meyerbaptiste requested changes Oct 2, 2017

View reviewed changes

soyuka reviewed Oct 16, 2017

View reviewed changes

soyuka approved these changes Oct 16, 2017

View reviewed changes

soyuka added the [RFR] Ready for Review label Nov 6, 2017

soyuka reviewed Nov 6, 2017

View reviewed changes

vincentchalamon added 7 commits November 7, 2017 09:35

Fix #1246 after merge

159572b

1246: allow default order on associations properties

93ac012

Fix review

b8eb3a0

Fix travis

bb8dc16

Fix review

09b6cfb

Fix PHPStan error

82218e8

Fix unit test

b42e46f

dunglas requested changes Nov 9, 2017

View reviewed changes

Fix review

c82dd82

dunglas approved these changes Nov 9, 2017

View reviewed changes

soyuka approved these changes Nov 9, 2017

View reviewed changes

dunglas merged commit 5d54089 into api-platform:2.1 Nov 9, 2017

vincentchalamon deleted the issues/1246 branch November 9, 2017 13:24

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Fix api-platform#1246 (api-platform#1324)

df65fa1

	protected function addJoinOnce(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $association): string
	{
	$join = $this->getExistingJoin($queryBuilder, $alias, $association);

	if (null === $join) {
	$associationAlias = $queryNameGenerator->generateJoinAlias($association);

	if (true === QueryChecker::hasLeftJoin($queryBuilder)) {
	$queryBuilder
	->leftJoin(sprintf('%s.%s', $alias, $association), $associationAlias);
	} else {
	$queryBuilder
	->innerJoin(sprintf('%s.%s', $alias, $association), $associationAlias);
	}
	} else {
	$associationAlias = $join->getAlias();
	}

	return $associationAlias;
	}

Uh oh!

[RFR] Issues/1246 #1324

[RFR] Issues/1246 #1324

Uh oh!

Conversation

vincentchalamon commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste left a comment

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!

vincentchalamon commented Oct 16, 2017

Uh oh!

soyuka commented Oct 16, 2017

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!

soyuka commented Nov 6, 2017

Uh oh!

vincentchalamon commented Nov 6, 2017

Uh oh!

soyuka commented Nov 6, 2017

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!

soyuka commented Nov 7, 2017

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

vincentchalamon commented Aug 22, 2017 •

edited

Loading