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

Identity in *-to-one association builder #1229

Conversation

guiwoda
Copy link
Contributor

@guiwoda guiwoda commented Dec 23, 2014

Added the ability to set identity with associations through the ClassMetadataBuilder / AssociationBuilder classes.

Added multiple tests to ensure that all association scenarios behave as expected.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3461

We use Jira to track the state of pull requests and the versions they got
included in.

@guiwoda guiwoda changed the title Identity in onetoone association builder Identity in *-to-one association builder Dec 23, 2014
@Ocramius Ocramius self-assigned this Dec 23, 2014
@Ocramius
Copy link
Member

@guiwoda the alignment in the code is messed up. Also, please only test bits relevant to your changes.

->build()
);

$this->assertEquals(array('groups' => array (
Copy link
Member

Choose a reason for hiding this comment

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

This assertion is too fragile

@guiwoda
Copy link
Contributor Author

guiwoda commented Jan 15, 2015

@Ocramius you mean in the tests?
I copy-pasted almost everything from previous tests. Could you point exactly what alignments are messed up? Or how would you do a less fragile assertion?

Also, all the tests I commited are related to my change. Please review it again if that's not clear.

@Ocramius
Copy link
Member

I copy-pasted almost everything from previous tests

Please build smaller assertions then, checking things such as $this->assertSame(['foo' => 'bar'], $metadata->getIdentifier());

@Ocramius
Copy link
Member

Poking @beberlei for a decision on whether we want this or not

@Ocramius
Copy link
Member

Decided to merge, but will do as soon as I have free time to check it carefully.

@Ocramius
Copy link
Member

Will merge as-is. If the tests are too fragile, we can reduce their scope later on.

Ocramius added a commit that referenced this pull request Feb 16, 2015
@Ocramius Ocramius closed this in a13143b Feb 16, 2015
@Ocramius
Copy link
Member

Merged after a rebase at a13143b

@beberlei
Copy link
Member

Please see b3a6fb7 for some changes to this behavior.

@guiwoda
Copy link
Contributor Author

guiwoda commented Mar 18, 2015

Great! I always found the "is" syntax confusing as a setter.

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

Successfully merging this pull request may close these issues.

None yet

4 participants