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.

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

[DDC-2794] Arbitrary Join count walkers solution #1092

Merged
merged 7 commits into from
Sep 22, 2014
Merged

[DDC-2794] Arbitrary Join count walkers solution #1092

merged 7 commits into from
Sep 22, 2014

Conversation

birko
Copy link
Contributor

@birko birko commented Jul 30, 2014

Possible solution for Arbitrary Join problem in pagination count
walkers:
https://groups.google.com/forum/#!topic/doctrine-user/rpPYCDNKOU8

Added a condition to test query component against SelectStatement from
clause

Possible solution for Arbitrary Join problem in pagination count
walkers:
https://groups.google.com/forum/#!topic/doctrine-user/rpPYCDNKOU8

Added a condition to test query component against  SelectStatement from
clause
@Ocramius
Copy link
Member

@birko an sql generation test is highly required here...

@birko
Copy link
Contributor Author

birko commented Jul 30, 2014

@Ocramius Indeed, Will be working on it

František Bereň added 4 commits July 30, 2014 15:55
changed root selection in Walkers from looping queryComponents to using
$AST->fromClause as other walkers have
Missing  braces and selected fields
@birko
Copy link
Contributor Author

birko commented Jul 31, 2014

Hope this test are sufficient in this PR

@Ocramius
Copy link
Member

@guilhermeblanco could you review this? Looks sane to me, and it would allow using the paginator with arbitrary joins :-)

$queryComponents = $this->_getQueryComponents();
// Get the root entity and alias from the AST fromClause
$from = $AST->fromClause->identificationVariableDeclarations;
if (count($from) > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

empty newline before this line

@Ocramius
Copy link
Member

Looks good, and the code is even simpler! There is a minor code duplication (when fetching the root component), but fixing it would require too much over-engineering here.

👍

@stof
Copy link
Member

stof commented Aug 20, 2014

👍

@stof
Copy link
Member

stof commented Aug 20, 2014

note that the related issue is http://www.doctrine-project.org/jira/browse/DDC-2794

@deeky666 deeky666 changed the title Arbitrary Join count walkers solution [DDC-2794] Arbitrary Join count walkers solution Aug 20, 2014
@birko
Copy link
Contributor Author

birko commented Aug 26, 2014

@Ocramius I will update the code according your comments. I was on vacation this week

$queryComponents = $this->_getQueryComponents();
// Get the root entity and alias from the AST fromClause
$from = $AST->fromClause->identificationVariableDeclarations;
$rootAlias = $from[0]->rangeVariableDeclaration->aliasIdentificationVariable;
Copy link
Member

Choose a reason for hiding this comment

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

The AST doesn't ensure that the array is indexed starting from 0, so this should probably be reset($from)

@Ocramius
Copy link
Member

This seems mergeable to me, except for my nitpicking.

@birko can you just verify if any docs requires updating?

@birko
Copy link
Contributor Author

birko commented Aug 31, 2014

@Ocramius will check

changed usage $from[0] according suggestion from @Ocramius  to use array
reset function
@birko
Copy link
Contributor Author

birko commented Sep 2, 2014

I looked slightly into the docs but did not find any reference that pagination is not working with Arbitrary joins. so I think the docs are ok

@birko
Copy link
Contributor Author

birko commented Sep 22, 2014

Hmm any progress what should I fix to get this merged?

@Ocramius Ocramius self-assigned this Sep 22, 2014
Ocramius added a commit that referenced this pull request Sep 22, 2014
[DDC-2794] Arbitrary Join count walkers solution
@Ocramius Ocramius merged commit 3f8865c into doctrine:master Sep 22, 2014
@Ocramius
Copy link
Member

@birko thanks for the reminder, merged! Thanks for the clean code as well :-)

@birko
Copy link
Contributor Author

birko commented Sep 23, 2014

@Ocramius No problem. Thanks for your hints as well :)

@adrienrusso
Copy link

@birko Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants