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

NamingStrategy::joinTableName not being called #5655

Closed
wants to merge 2 commits into from

Conversation

basz
Copy link

@basz basz commented Feb 7, 2016

This problem exists for M2M without a tableName (because a NamingStrategy may take care of the table name) and XML Drivers

Explanation:

Although a joinTable MUST have an tableName eventually, it is actually valid to have it not declared IF a naming strategy would generate one (always true if not defined (null)).

However this XML driver will will parse an omitted declaration as an empty string. The ClassMetadataInfo Class is expecting an unset value or it will break.

ClassMetadataInfo will show a Notice: Uninitialized string offset: 0 which makes sense if you look at

// remember $mapping['joinTable']['name'] is now "";
if (isset($mapping['joinTable']['name']) && $mapping['joinTable']['name'][0] === '`') {

see

More importantly the (default) naming strategy will now never be called and the resulting ClassMetadataInfo has no table name defined, which results in a 'Invalid table name specified' Exception

if ( ! isset($mapping['joinTable']['name'])) {
    $mapping['joinTable']['name'] = $this->namingStrategy->joinTableName($mapping['sourceEntity'], $mapping['targetEntity'], $mapping['fieldName']);
}

see

Please tell me if this is something we can merge without further proof, I don't really understand how to add a test case for it (I have looked at the existing cases, but... pfft)

This problem exists for M2M without a tableName (because a NamingStrategy may take care of the table name) and XML Drivers

Explanation:

Although a joinTable MUST have an tableName eventually, it is actually valid to have it not declared IF a naming strategy would generate one (always true if not defined (null)).

However this XML driver will will parse an omitted declaration as an empty string. The ClassMetadataInfo Class is expecting an unset value or it will break.

ClassMetadataInfo will show a `Notice: Uninitialized string offset: 0` which makes sense if you look at

(see)[https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L1521]
```
// with $mapping['joinTable']['name'] = '';
 if (isset($mapping['joinTable']['name']) && $mapping['joinTable']['name'][0] === '`') {
```

More importantly the (default) naming strategy will now never be called and the resulting ClassMetadataInfo has no table name defined, which results in a 'Invalid table name specified' Exception

(see)[https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L1703-L1705]
```
            if ( ! isset($mapping['joinTable']['name'])) {
                $mapping['joinTable']['name'] = $this->namingStrategy->joinTableName($mapping['sourceEntity'], $mapping['targetEntity'], $mapping['fieldName']);
            }
```

Please tell me if this is something we can merge without further proof, I don't really understand how to add a test case for it (I have looked at the existing cases, but... pfft)
@Ocramius
Copy link
Member

Ocramius commented Feb 8, 2016

@basz please refer to https://github.com/doctrine/doctrine2/blob/788143dc0313c7522820ebf6057f73881e7190a3/tests/Doctrine/Tests/ORM/Mapping/XmlMappingDriverTest.php

You basically want to create a <join-table/> without a name attribute. This seems invalid though, as the name attribute is required as per XSD specification: https://github.com/doctrine/doctrine2/blob/788143dc0313c7522820ebf6057f73881e7190a3/doctrine-mapping.xsd#L453

@basz
Copy link
Author

basz commented Feb 8, 2016

This seems invalid though, as the name attribute is required as per XSD specification: https://github.com/doctrine/doctrine2/blob/788143dc0313c7522820ebf6057f73881e7190a3/doctrine-mapping.xsd#L453

Hm I did not think about that. Would an empty string be valid according to the xsd?

Otherwise Naming strategies will never be able to kick in for xml schemes

@Ocramius
Copy link
Member

Ocramius commented Feb 8, 2016

Otherwise Naming strategies will never be able to kick in for xml schemes

A join table can be omitted

@basz
Copy link
Author

basz commented Feb 8, 2016

Not if you want to specify other properties. I would argue that the doctrine-mapping.xsd is incorrect on the requirement of the name attribute since doctrine supported NamingStrategies...

This should work IMHO

  • join-table name should be autogenerated
  • join-column name should be autogenerated
      <join-table>
        <join-columns>
          <join-column referenced-column-name="id" nullable="false" on-delete="CASCADE"/>
        </join-columns>
        <inverse-join-columns>
          <join-column referenced-column-name="id" nullable="false" on-delete="CASCADE"/>
        </inverse-join-columns>
      </join-table>

@Ocramius
Copy link
Member

Ocramius commented Feb 8, 2016

@basz changing the requirement could be done, but is it worth doing?

@basz
Copy link
Author

basz commented Feb 8, 2016

Probably not. but if it is not accurate, why not?

For example skipper will try to generate a join column name when omitted. When asked to stop doing they will probably say it a requirement by declaration.

Op 8 feb. 2016 om 20:51 heeft Marco Pivetta notifications@github.com het volgende geschreven:
@basz changing the requirement could be done, but is it worth doing?


Reply to this email directly or view it on GitHub.

Base automatically changed from master to old-prototype-3.x February 23, 2021 08:18
@basz basz closed this Oct 28, 2022
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

3 participants