ReferenceMany: insert an empty array

[khaperets]

Q	A
Type	improvement
BC Break	yes
Fixed issues	#1195

Summary

See #1195

[khaperets]

#1624

[malarzm]

Glad you decided to be "someone to pick that PR up"! 🎉

Unfortunately we still need to consider #1624 (comment), even more than before, after 6 more years of previous behaviour and no 3.0 on horizon.

[khaperets]

@malarzm Added optional parameter insertEmptyArray

[khaperets]

@malarzm ping :)

[malarzm]

Thanks for the patience while I've been on vacations :) This is a good start, however there are few things I need you to do.. First things first, please rebase your work atop current 2.6.x branch, some of the errors will go away. Other things (in random order)

1. Same feature should be made available for EmbedMany association
2. XML mapping support is missing, you can take a look here https://github.com/doctrine/mongodb-odm/blob/2.6.x/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php#L454 how bool-ish attributes should be handled. XSD (https://github.com/doctrine/mongodb-odm/blob/2.6.x/doctrine-mongo-mapping.xsd) file shall be updated accordingly
3. New option should be added to https://github.com/doctrine/mongodb-odm/blob/2.6.x/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php#L155. Combined with 1 this should effectively make ?? false construct not needed in your code
4. How about naming the feature storeEmptyArray? Right now we're strictly talking about inserting document, but there's more to consider like clearing a persistent collection, which right now issues a $unset query here https://github.com/doctrine/mongodb-odm/blob/2.6.x/lib/Doctrine/ODM/MongoDB/Persisters/CollectionPersister.php#L82
5. The documentation should be updated

[khaperets]

Thanks for your detailed answer. I'll get to it soon.
P.S. I hope you had a good rest :)

[khaperets]

1. Done
2. Done
3. Done
4. Renamed the new functionality.
5. Done

[khaperets]

@malarzm ping :)

[malarzm]

Thank you very much for changes, they're looking A-OK! We're almost there, one last thing we need to do is increase test coverage to reflect your latest changes as right now we might accidentally break stuff from CollectionPersister and don't know about it 🙈 These tests should be included:

1. At least one property that AbstractDriverTestCase is asserting on should have storeEmptyArray set to true - this way we'll know the support in attributes and XML is on par
2. Take a look at https://github.com/doctrine/mongodb-odm/blob/2.6.x/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php and add a test or two there - we want to make sure that after the collection is emptied, an empty array is in fact stored. No need to test nested paths and whatnot as that's already covered

We'll make sure to include this in 2.6.0 (cc @alcaeus)

[khaperets]

1. Done
2. Done

Thank you very much for the code review!

[malarzm]

One last thing I've caught, sorry it's that late in the process. Also if you'd been fixing that, mind squashing commits so there's just 1?

[malarzm]

Caught this with a last glance, don't we have a bug here? An empty collection using addToSet strategy will not have an empty array stored. A test may prove me wrong 👀

[khaperets]

Fixed, added a case to test

[khaperets]

One last thing I've caught, sorry it's that late in the process. Also if you'd been fixing that, mind squashing commits so there's just 1?

Done

[malarzm]

Awesome! Thank you @khaperets very much for sticking with us (and me not replying promptly) and seeing this to an end 🍻

[khaperets]

@malarzm Many thanks for your help!