phpro / zf-doctrine-hydration-module

Configurable Doctrine hydrators for ZF2
18 stars 33 forks source link

Hotfix/avoid sending null embedded objects to hydrators #30

Closed matwright closed 8 years ago

matwright commented 8 years ago

EmbedField Strategy extract() method should only accept object values. When an embed field is not set or null a php null value is passed to this method and passed onto the DoctrineHydrator extract method that expects an object, this causes Exceptions to be thrown further down the line when the ClassMethods class is invoked using a get_class function call on the null value.

https://github.com/phpro/zf-doctrine-hydration-module/issues/29

veewee commented 8 years ago

Thanks for reporting the bug. Can you add the test case provided in the issue and fix the CS issue? I guess this one needs to be patched to 2.0 and 1.0 ?

matwright commented 8 years ago

Fixed CS, the build is failing one of the automated checks but looks like its an environment error with mongo not code related ?

I also updated the test to assert that the getEmbed returns null when not set.

Will send a PR for version 1.0 also.

veewee commented 8 years ago

The error in Travis is related to php-cs-fixer. Maybe the minimum version should just be bumped to a newer version since it is just a dev dependency.

{"files":[{"name":"\/home\/travis\/build\/phpro\/zf-doctrine-hydration-module\/src\/Hydrator\/ODM\/MongoDB\/Strategy\/EmbeddedField.php","appliedFixers":["braces"]}],"memory":7.5,"time":{"total":0.103}}

The warning about mongo is 'normal'. The doctrine-odm repository also has this problem.

matwright commented 8 years ago

Yes I have the mongo warning here too. But the cs-fixer is strange, why is it failing only on that one build for that specific file ?

veewee commented 8 years ago

That build is running with DEPENDENCIES=low which will run composer update --prefer-lowest. So it might be due to an issue in the cs fixer or a fixer that works in a different way then the old version.

matwright commented 8 years ago

yes that would explain.

veewee commented 8 years ago

Thanks!