Skip to content
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.

Sign up for GitHub

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

Fix join queries #828

Closed
wants to merge 5 commits into from
Closed

Fix join queries #828

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9449dfb
use doctrine paginator to get results
patgrudniewski Feb 11, 2016
bdb52c7
Merge pull request #1 from francuz91/fix-joinQueries
patgrudniewski Feb 11, 2016
5712a97
check if join is fetch join
patgrudniewski Feb 12, 2016
a923e2e
Merge pull request #2 from francuz91/fix-joinQueries
patgrudniewski Feb 12, 2016
02bfa32
empty join table check
patgrudniewski Feb 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion Grid/Source/Entity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*
* (c) Abhoryo <abhoryo@free.fr>
* (c) Stanislav Turza
* (c) Patryk Grudniewski <patgrudniewski@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
Expand All @@ -19,6 +20,7 @@
use Doctrine\ORM\Query;
use Doctrine\ORM\QueryBuilder;
use Symfony\Component\HttpKernel\Kernel;
use Doctrine\ORM\Query\Expr\Join;
use Doctrine\ORM\Query\ResultSetMapping;
use Doctrine\ORM\Tools\Pagination\CountWalker;
use Doctrine\ORM\Tools\Pagination\Paginator;
Expand Down Expand Up @@ -451,7 +453,7 @@ public function execute($columns, $page = 0, $limit = 0, $maxResults = null, $gr

//call overridden prepareQuery or associated closure
$this->prepareQuery($this->query);
$hasJoin = !empty($this->query->getDqlPart('join'));
$hasJoin = $this->checkIfQueryHasFetchJoin($this->query);

$query = $this->query->getQuery();
foreach ($this->hints as $hintKey => $hintValue) {
Expand Down Expand Up @@ -779,4 +781,20 @@ public function getTableAlias()
{
return $this->tableAlias;
}

/**
* @param QueryBuilder $qb
* @return boolean
*/
protected function checkIfQueryHasFetchJoin(QueryBuilder $qb)
{
$join = $qb->getDqlPart('join');
foreach ($join[$this->getTableAlias()] as $join) {
if ($join->getJoinType() === Join::INNER_JOIN) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The join type doesn't matter. The check should be if (joined entity is also present in select) imo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that join type matter. I cannot explained that, because I wrote that in February, but I'm pretty sure that it was caused by bug found by my former team's QA.

There was some kind of uncompatibility with Doctrine's paginator when there was just
$hasJoin = !empty($this->query->getDqlPart('join')). I believe that Paginator couldn't been created successfully when new Paginator($query, true) constructor was called for not fetch joined query.

I'm sorry that I cannot tell you more but I haven't been working at project I was using APYDataGridBundle for 4 months.

For more details you have to ask Pixers, they forked my repo and I believe there're still using APYDataGridBundle. Also their QA team had found bug I fixed here :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you only check for inner joins here?

I tested this and it all works fine unless you have inner joins. In my case the QueryBuilder is built with only left joins, which then skips this check.

The following would solve it;

if ($join->getJoinType() === Join::INNER_JOIN || $join->getJoinType() === Join::LEFT_JOIN) {
    return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rvanlaarhoven cloud you send a push to this PR.

return true;
}
}

return false;
}
}