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

Merged
merged 1 commit into from Jun 8, 2016

Projects

None yet

4 participants

@tPl0ch
tPl0ch commented Jun 7, 2016 edited

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 from `YamlExporter` should only inspect `joinColumns` for owning side in bi-directional OneToOne to Exporters should only inspect `joinColumns` for owning side in bi-directional OneToOne Jun 8, 2016
@nickdani
nickdani commented Jun 8, 2016 edited

👍

@grimskin
grimskin commented Jun 8, 2016

👍

@Ocramius Ocramius commented on an outdated diff Jun 8, 2016
lib/Doctrine/ORM/Tools/Export/Driver/PhpExporter.php
@@ -114,10 +114,11 @@ public function exportClassMetadata(ClassMetadataInfo $metadata)
if ($associationMapping['type'] & ClassMetadataInfo::TO_ONE) {
$method = 'mapOneToOne';
+ $joinColumns = $associationMapping['isOwningSide'] === true ? $associationMapping['joinColumns'] : array();
@Ocramius
Ocramius Jun 8, 2016 Member

[] 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 commented on an outdated diff Jun 8, 2016
lib/Doctrine/ORM/Tools/Export/Driver/YamlExporter.php
@@ -163,7 +163,7 @@ public function exportClassMetadata(ClassMetadataInfo $metadata)
}
if ($associationMapping['type'] & ClassMetadataInfo::TO_ONE) {
- $joinColumns = $associationMapping['joinColumns'];
+ $joinColumns = $associationMapping['isOwningSide'] === true ? $associationMapping['joinColumns'] : array();
@Ocramius
Ocramius Jun 8, 2016 Member

Same as above with === true (not needed)

@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
Thomas Ploch Exporters should only inspect `joinColumns` for owning side in bi-dir…
…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
ea788fb
@Ocramius Ocramius merged commit ea788fb into doctrine:master Jun 8, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@Ocramius Ocramius added a commit that referenced this pull request Jun 8, 2016
@Ocramius Ocramius Merge branch 'fix/#5858-yaml-exporter-should-only-introspect-join-col…
…umn-on-owning-association-side'

Close #5858
81fe6a8
@Ocramius Ocramius added a commit that referenced this pull request Jun 8, 2016
@Ocramius Ocramius Merge branch 'fix/#5858-yaml-exporter-should-only-introspect-join-col…
…umn-on-owning-association-side-2.5' into 2.5

Close #5858
0d93461
@Ocramius
Member
Ocramius commented Jun 8, 2016
@tPl0ch
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
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