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

Alias for Lookup with Relationship #2172

Merged
merged 26 commits into from
Mar 31, 2020

Conversation

josemiguelq
Copy link
Contributor

@josemiguelq josemiguelq commented Mar 26, 2020

Q A
Type improvement
BC Break no
Fixed issues

Summary

According to the documentation of Mongo the as can't be null because contains the matching documents from the from collection.
(https://docs.mongodb.com/manual/reference/operator/aggregation/lookup/#lookup-join-as).

To prevent error the keys will be the same of the Documents using "mappedBy" and the foreignKey will be the same of attribute "name".

@josemiguelq josemiguelq reopened this Mar 26, 2020
@josemiguelq josemiguelq changed the title Alias for Lookup Alias for Lookup with Relationship Mar 26, 2020
@alcaeus
Copy link
Member

alcaeus commented Mar 26, 2020

Thanks for your contribution! This needs a little more work:

  1. Please give your commit a good title. "Use field name as default alias in Lookup stage" would be a good title.
  2. Rebase your commit on the 1.3.x branch and re-target the pull request to that branch, as this is a bug we should fix in the oldest supported version
  3. Please add a test for it, there are a couple of tests for the Lookup stage already.

Please let me know if you need help with any of the above!

@josemiguelq
Copy link
Contributor Author

Thanks for your contribution! This needs a little more work:

  1. Please give your commit a good title. "Use field name as default alias in Lookup stage" would be a good title.
  2. Rebase your commit on the 1.3.x branch and re-target the pull request to that branch, as this is a bug we should fix in the oldest supported version
  3. Please add a test for it, there are a couple of tests for the Lookup stage already.

Please let me know if you need help with any of the above!

Thanks @alcaeus , I'll do the corrections that you pointed.

@josemiguelq josemiguelq changed the base branch from master to 1.3.x March 27, 2020 14:17
@josemiguelq josemiguelq changed the base branch from 1.3.x to master March 27, 2020 14:23
@josemiguelq josemiguelq changed the base branch from master to 1.3.x March 27, 2020 14:29
@josemiguelq
Copy link
Contributor Author

@alcaeus , checks my updates please?

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

There are lots of unrelated changes in this PR regarding indentation. Please refrain from changing the coding style on unrelated changes. If your IDE is doing this automatically, please tell it to stop doing that 😉

As for the tests, those look good so far. The two classes you introduced aren't necessary. For the mapping changes, was the storeAs attribute required to make tests pass? Inverse references shouldn't care about their own storeAs, as they're defined by the storeAs value of the owning side. If the test fails without this mapping, this points to a different problem with the lookup implementation (which I'd have to debug separately).

tests/Documents/ReferenceCampaign.php Outdated Show resolved Hide resolved
tests/Documents/Campaign.php Outdated Show resolved Hide resolved
tests/Documents/User.php Outdated Show resolved Hide resolved
@josemiguelq
Copy link
Contributor Author

josemiguelq commented Mar 30, 2020

@alcaeus , request changes done. Thanks for the review and advice.

@alcaeus
Copy link
Member

alcaeus commented Mar 30, 2020

@josemiguelq there are some more indentation changes that you haven't changed back. Please go through those and make sure to not have any unrelated changes in. The rest looks good 👍

@josemiguelq
Copy link
Contributor Author

@josemiguelq there are some more indentation changes that you haven't changed back. Please go through those and make sure to not have any unrelated changes in. The rest looks good 👍

Sorry @alcaeus, indentation changes done. Its IDE default behavior

@alcaeus alcaeus self-assigned this Mar 31, 2020
@alcaeus alcaeus added the Bug label Mar 31, 2020
@alcaeus alcaeus added this to the 1.3.7 milestone Mar 31, 2020
@alcaeus
Copy link
Member

alcaeus commented Mar 31, 2020

Thanks @josemiguelq!

Note: travis-ci completed successfully but did not correctly report the status to GitHub.

@alcaeus alcaeus merged commit 1c6fade into doctrine:1.3.x Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants