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

+ Index name for OneToOne relation #1498

Open
wants to merge 1 commit into
base: old-prototype-3.x
Choose a base branch
from

Conversation

imldev
Copy link

@imldev imldev commented Sep 1, 2015

No description provided.

@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-3883

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

@@ -340,6 +340,7 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$mapping['cascade'] = $oneToOneAnnot->cascade;
$mapping['orphanRemoval'] = $oneToOneAnnot->orphanRemoval;
$mapping['fetch'] = $this->getFetchMode($className, $oneToOneAnnot->fetch);
$mapping['indexName'] = $oneToOneAnnot->indexName;
Copy link
Member

Choose a reason for hiding this comment

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

@deeky666 poke

Copy link
Member

Choose a reason for hiding this comment

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

I think the less we mess with implicit indexes the better - a better solution would be to define an @Index on the table that satisfies the FK index. Not sure if the TableDiff will prevent creation of the FK index if it's exactly satisfied by an explicit index though.

Copy link
Member

Choose a reason for hiding this comment

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

@zeroedin-bill I disagree here. It's a lot easier and less error prone to have an attribute on the association simply for the index name. Also it relates a lot better to have it at this place rather than loosely in the table section. You can still do your own @Index definitions to overwrite implicit indexes if you want to. But simply being able to change the index name without having to clutter the definition with another @Index makes totally sense IMO.

@Ocramius
Copy link
Member

Ocramius commented Sep 1, 2015

@imldev needs test cases, as well as patches for the XML and YML driver (if @deeky666 thinks this is acceptable)

@deeky666
Copy link
Member

deeky666 commented Sep 7, 2015

@imldev @Ocramius I am fine with this kind of approach, even proposed something like the in the past before. But this PR needs to cover O2M / M2O + maybe M2M (if somehow possible), too. Also I am missing schema tool adjustments here too for the change to have any effect?

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.

5 participants