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

Exporters should only inspect joinColumns for owning side in bi-directional OneToOne #5858

Merged
merged 1 commit into from
Jun 8, 2016

Conversation

tPl0ch
Copy link
Contributor

@tPl0ch tPl0ch commented Jun 7, 2016

After having run the YamlExporter in one of our projects there was a weird behavior when a bi-directional OneToOne association was defined.

I managed to replicate the behavior with adding an inversed bi-directional mapping to the test metadata:

Doctrine.Tests.ORM.Tools.Export.User.dcm.yml

#snip
  oneToOne:
    address:
      targetEntity: Doctrine\Tests\ORM\Tools\Export\Address
      joinColumn:
        name: address_id
        referencedColumnName: id
        onDelete: CASCADE
      cascade: [ persist ]
      inversedBy: user
      orphanRemoval: true
      fetch: EAGER
    cart:
      targetEntity: Doctrine\Tests\ORM\Tools\Export\Cart
      mappedBy: user
      cascade: [ remove ]
#snip
$ ./vendor/bin/phpunit -vv tests/Doctrine/Tests/ORM/Tools/Export/YamlClassMetadataExporterTest.php
PHPUnit 4.8.26-3-ga973e60 by Sebastian Bergmann and contributors.

Runtime:    PHP 5.6.22-1~dotdeb+7.1 with Xdebug 2.3.3
Configuration:  /vagrant/libs/doctrine2/phpunit.xml.dist

ESSSSSSSSSSSSSS

Time: 156 ms, Memory: 9.75MB

There was 1 error:

1) Doctrine\Tests\ORM\Tools\Export\YamlClassMetadataExporterTest::testExportDirectoryAndFilesAreCreated
Undefined index: joinColumns

/vagrant/libs/doctrine2/lib/Doctrine/ORM/Tools/Export/Driver/YamlExporter.php:166
/vagrant/libs/doctrine2/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php:138
/vagrant/libs/doctrine2/tests/Doctrine/Tests/ORM/Tools/Export/AbstractClassMetadataExporterTest.php:121

So the problem is that the joinColumn array index is not set for the inversed side but will still be inspected. After applying the check for $associationMapping['isOwningSide'] === true ? $associationMapping['joinColumns'] : array(); the export works as expected.

Doctrine.Tests.ORM.Tools.Export.ExportedUser.dcm.yml

# snip
    oneToOne:
        address:
            targetEntity: Doctrine\Tests\ORM\Tools\Export\Address
            cascade:
                - remove
                - persist
            fetch: EAGER
            mappedBy: null
            inversedBy: user
            joinColumns:
                address_id:
                    referencedColumnName: id
                    onDelete: CASCADE
            orphanRemoval: true
        cart:
            targetEntity: Doctrine\Tests\ORM\Tools\Export\Cart
            cascade:
                - remove
            fetch: LAZY
            mappedBy: user
            inversedBy: null
            joinColumns: {  }
            orphanRemoval: false
# snip

EDIT

Same applies to the PhpExporter when duplicating the test case to the test metadata:

$ ./vendor/bin/phpunit -vv tests/Doctrine/Tests/ORM/Tools/Export
PHPUnit 4.8.26-3-ga973e60 by Sebastian Bergmann and contributors.

Runtime:    PHP 5.6.22-1~dotdeb+7.1 with Xdebug 2.3.3
Configuration:  /vagrant/libs/doctrine2/phpunit.xml.dist

......S.......SESSSSSSSSSSSSSS......................S........

Time: 410 ms, Memory: 13.00MB

There was 1 error:

1) Doctrine\Tests\ORM\Tools\Export\PhpClassMetadataExporterTest::testExportDirectoryAndFilesAreCreated
Undefined index: joinColumns

/vagrant/libs/doctrine2/lib/Doctrine/ORM/Tools/Export/Driver/PhpExporter.php:120
/vagrant/libs/doctrine2/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php:138
/vagrant/libs/doctrine2/tests/Doctrine/Tests/ORM/Tools/Export/AbstractClassMetadataExporterTest.php:125

@tPl0ch tPl0ch changed the title YamlExporter should only inspect joinColumns for owning side in bi-directional OneToOne Exporters should only inspect joinColumns for owning side in bi-directional OneToOne Jun 8, 2016
@nickdani
Copy link

nickdani commented Jun 8, 2016

👍

1 similar comment
@grimskin
Copy link

grimskin commented Jun 8, 2016

👍

@@ -114,10 +114,11 @@ public function exportClassMetadata(ClassMetadataInfo $metadata)

if ($associationMapping['type'] & ClassMetadataInfo::TO_ONE) {
$method = 'mapOneToOne';
$joinColumns = $associationMapping['isOwningSide'] === true ? $associationMapping['joinColumns'] : array();
Copy link
Member

Choose a reason for hiding this comment

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

[] can be used here. Variable also not needed (can directly be inlined into line 121). === true also not needed, as the value is boolean

@Ocramius Ocramius added the Bug label Jun 8, 2016
@Ocramius Ocramius added this to the 2.5.5 milestone Jun 8, 2016
@Ocramius Ocramius self-assigned this Jun 8, 2016
…ectional OneToOne

rebased commits:

- Added test case for bi-directional OneToOne in YamlExporter
- Only inspect joinColumns for owning side in bi-directional OneToOne in YamlExporter
- Adding bi-directional test case without joinColumn to XmlExporter test
- Same testcase also applied to PhpExporter
- Fixing bi-directional issue in PhpExporter when inspecting joinColumns index
- Implemented @Ocramius suggestions
@Ocramius Ocramius merged commit ea788fb into doctrine:master Jun 8, 2016
Ocramius added a commit that referenced this pull request Jun 8, 2016
Ocramius added a commit that referenced this pull request Jun 8, 2016
…umn-on-owning-association-side-2.5' into 2.5

Close #5858
@Ocramius
Copy link
Member

Ocramius commented Jun 8, 2016

@tPl0ch merged, thanks!

2.5: 0d93461
master: 81fe6a8

@tPl0ch
Copy link
Contributor Author

tPl0ch commented Jun 8, 2016

@Ocramius There are a few test failures that I cannot explain though: https://travis-ci.org/doctrine/doctrine2/builds/136120071

@Ocramius
Copy link
Member

Ocramius commented Jun 8, 2016

Yeah, they are unrelated to this change though. Will have to check them

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.

4 participants