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

Handle mapping without mappedBy #6297

Merged
merged 1 commit into from
Aug 22, 2020

Conversation

VincentLanglet
Copy link
Member

Subject

I am targeting this branch, because BC.

Fix #6219 (comment).

Changelog

### Fixed
- ObjectManipulator::setObject does not throw an error anymore for DoctrineODM Embedded collections.

@VincentLanglet VincentLanglet requested a review from a team August 18, 2020 19:23
jordisala1991
jordisala1991 previously approved these changes Aug 19, 2020
@greg0ire
Copy link
Contributor

Please summarize the linked issue in the body of your commit message. We should be able to understand what this is about just by reading the git log.

@VincentLanglet
Copy link
Member Author

Please summarize the linked issue in the body of your commit message. We should be able to understand what this is about just by reading the git log.

Please enjoy my first commit with a body @greg0ire 😅

@greg0ire
Copy link
Contributor

Please enjoy my first commit with a body @greg0ire 😅

I am so proud of you!

@greg0ire
Copy link
Contributor

Oh, it's just a link… a summary would really be better IMO. I'm going to read the comment, then read the PR and try to really understand this, and then I will write the summary.

That attribute is not guaranteed to exist, that's for example the case
of @mongodb\EmbedMany

See sonata-project#6219 (comment)
@greg0ire
Copy link
Contributor

Things I changed in the commit message:

  1. the commit message subject is about the what. If you read it, then look at the changes, you should think "Indeed, that's what is being done here, not more, not less."
  2. the former commit message subject has been moved in the body, because it is about the why. I added an example so that it's less vague, and I kept the link so that people that want to see the discussion can.

@VincentLanglet
Copy link
Member Author

Things I changed in the commit message:

  1. the commit message subject is about the what. If you read it, then look at the changes, you should think "Indeed, that's what is being done here, not more, not less."
  2. the former commit message subject has been moved in the body, because it is about the why. I added an example so that it's less vague, and I kept the link so that people that want to see the discussion can.

Interesting.
I always tried to write the why in the commit.

If I add a null !== $foo check, I would have create a commit

Avoid error when $foo is null

Rather than, the captain obvious commit

Add null check

But I think I understand your point of view.
You want the what to find your commit in the history, then the why you understand the reason of the commit.

@VincentLanglet VincentLanglet requested a review from a team August 22, 2020 12:27
@jordisala1991 jordisala1991 merged commit 16eb5cf into sonata-project:3.x Aug 22, 2020
@jordisala1991
Copy link
Member

Thank you @VincentLanglet

@greg0ire
Copy link
Contributor

Avoid error when $foo is null

Yeah, and that would be a bit terse… I mean when can $foo be null? It's probably not super obvious since it was missed before, so it might deserve a sentence explaining how this can happen. Hard to do under 50 chars.

@VincentLanglet VincentLanglet deleted the fixMappedBy branch August 22, 2020 12:58
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.

3 participants