Skip to content

Added queries testing nested joins #177

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

Closed
wants to merge 5 commits into from
Closed

Added queries testing nested joins #177

wants to merge 5 commits into from

Conversation

shinyvision
Copy link

Added tests to check whether nested joins are working. These work with my nested join implementations in jackalope/jackalope-doctrine-dbal and phpcr/phpcr-utils

@lsmith77
Copy link
Member

@richard-fizz ok great .. now can you adjust your composer.json in jackalope/jackalope-doctrine-dbal#318 to use your branch in your repository (see also https://getcomposer.org/doc/05-repositories.md#using-private-repositories)? this way we can automatically verify that your code passes the tests.

ideally you would then also create a similar PR for https://github.com/jackalope/jackalope-jackrabbit

once we have confirmed that your tests pass with both we will merge this PR and create a new tag. you will then have to update your PRs again to use this new tag instead of your own branch.

@shinyvision
Copy link
Author

Pushed the modified composer.json to jackalope/jackalope-doctrine-dbal#318.

@dbu
Copy link
Member

dbu commented Feb 12, 2016

can you please add one test that is actually run against the repository? the tests you add so far are good, they test the parser and converter. but we should have one in QuerySql2OperationsTest, along the lines of testQueryJoinWithAlias https://github.com/phpcr/phpcr-api-tests/blob/master/tests/Query/QuerySql2OperationsTest.php#L168 - maybe call it testQueryJoinNested ? (no need to test all the other join situations with nested, just one more that does a nested join)

@shinyvision
Copy link
Author

Done. Did find one more issue using the new test and solved it right away. Should work properly now.

@dbu
Copy link
Member

dbu commented Feb 12, 2016

the tests work with jackalope-jackrabbit as well. can you please squash the commits into one? then i can merge this and tag a version that we can use in jackalope

Richard Snijders added 2 commits February 12, 2016 17:15
@shinyvision
Copy link
Author

Did I squash the commits properly...? Never done it before.

@dbu
Copy link
Member

dbu commented Feb 12, 2016

no, not really ;-)
but i do the squash on the command line. what i did is:

git log # look for the first commit that is already in master
git rebase -i 4816315a3cbf5773fc55c53cdac135aeffff1957 # commit id i found in the log
# replace the `pick` by `squash` for all but the first line, save and exit
# look at the commit messages that are shown, only keep the relevant commit comment

now i have a clean history. if i was the author, i would now git push -f origin HEAD to update my fork branch with the squashed commits. with -f git would give me an error as my local history is different from upstream. git pull or git pull --rebase is not the right thing to do now, as that would make the squashing pointless. we do want to get rid of the detailed history of the branch we squashed...

as it is, i merged the PR on the command line and pushed as 772f2df

@dbu dbu closed this Feb 12, 2016
@lsmith77 lsmith77 removed the wip/poc label Feb 12, 2016
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.

3 participants