Closed healerz closed 6 years ago
malarzm
commented
6 years ago @healerz We have a default strategy set in place for embed many fields (example in the annotation and IIRC drivers also use this) so I don't expect the notice to arise. Could you please reproduce this as a failing test case?
alcaeus
commented
6 years ago my opinion there is a isset($mapping['strategy']) check missing
A failing test is worth more than a thousand opinions. Please provide one that fails so we know the proposed change fixes it.
Due to missing permissions, i didn't opened a PR.
You can fork the repo and create a pull request.
Ninja edit: damn you @malarzm!
healerz
commented
6 years ago @malarzm Yes there is a default strategy, and it works fine. Apart from the mentioned line which assumes that the "strategy" key of the mapping array is set, through checking if it is an atomic strategy ;)
@alcaeus Thank you for the input
I opened a PR, containing both, the test and the fix: https://github.com/doctrine/mongodb-odm/pull/1681 (without the fix, the test fails)
malarzm
commented
6 years ago Closing as #1681 was merged
In my opinion there is a isset($mapping['strategy']) check missing: https://github.com/doctrine/mongodb-odm/blob/957f4199c417aec34e489d297563afa34bd5b174/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php#L1433
My suggestion is to change this line to the following:
if ($this->isEmbeddedDocument && $mapping['type'] === 'many' && isset($mapping['strategy']) && CollectionHelper::isAtomic($mapping['strategy'])) {The missing isset() check results in enforcing the user to set a storage strategy in case of adding a "many embedded document" mapping. Else the user will end up with notices.
Due to missing permissions, i didn't opened a PR.