Fix collectionClass not working with the Yaml or Xml drivers. #1458
Conversation
| 'type' => $type, | ||
| 'embedded' => true, | ||
| 'targetDocument' => isset($attributes['target-document']) ? (string) $attributes['target-document'] : null, | ||
| 'collectionClass' => isset($attributes['collectionClass']) ? (string) $attributes['collectionClass'] : null, |
There was a problem hiding this comment.
Should we use collection-class to be consistent?
|
@trq thanks a bunch for catching this! I have no idea why have I forgotten about them... Apart from my 2 comments above could you also please
|
|
No worries, thanks for the feedback. I'll make these changes and update shortly. |
|
I'm a little lost with how/where to test the mapping on the XmlDriver. Any advice much appreciated. Thanks. |
| @@ -109,6 +109,7 @@ | |||
| <xs:element name="default-discriminator-value" type="odm:default-discriminator-value" minOccurs="0"/> | |||
| </xs:sequence> | |||
| <xs:attribute name="target-document" type="xs:string" /> | |||
| <xs:attribute name="collection-class" type="xs:string" /> | |||
There was a problem hiding this comment.
There seem to be collectionClass in line 115/116 (and 168/170) on which I can't comment, could you remove that?
|
As for XML tests you could tweak this AbstractMappingDriverTest which is extended by each driver type we have, here's XML mapping used by the test: https://github.com/doctrine/mongodb-odm/blob/master/tests/Doctrine/ODM/MongoDB/Tests/Mapping/xml/Doctrine.ODM.MongoDB.Tests.Mapping.AbstractMappingDriverUser.dcm.xml and probably you'll need to update tested YAML and annotation one too :) |
|
I've added some more tests, hopefully not making too much mess. I still have one failing test that I'll look at, but I would appreciate some help if anyone want's to take a look. |
| $this->assertArrayHasKey($key, $class->fieldMappings[$embeddedField]); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public function testGetAssociationCollectionClass() | ||
| { | ||
| $className = __NAMESPACE__.'\AbstractMappingDriverAlternateUser'; |
There was a problem hiding this comment.
I think you wanted to use AbstractMappingDriverUser as that's the class you changed mapping for
There was a problem hiding this comment.
P.S. sorry for such long break :)
|
@trq thanks for making tests green again! I should start merging stuff on Monday/Tuesday as I'm off for the weekend :) |
|
No worries, thanks for your help. Do you want me to squash this PR into a single commit or is there anything On Saturday, 23 July 2016, Maciej Malarz [email protected] wrote:
|
|
I can squash commits and merge manually, no worries :) |
The custom collections recently implemented in #1385 looks like it was never working with the Yaml (or Xml) drivers.
This PR addresses that issue.