Add custom doctrine types before mapping them #23
Conversation
|
HHVM failure is not because of this fix |
| } | ||
|
|
||
| $applicationConfig = $container->has('config') ? $container->get('config') : []; | ||
| $doctrineConfig = array_key_exists('doctrine', $applicationConfig) ? $applicationConfig['doctrine'] : []; |
There was a problem hiding this comment.
$doctrineConfig = $applicationConfig['doctrine'] ?? []
| return; | ||
| } | ||
|
|
||
| $applicationConfig = $container->has('config') ? $container->get('config') : []; |
There was a problem hiding this comment.
Just let it crash if 'config' is not set - this is a very likely failure scenario that, if made silent, can lead to a lot of hair pulling
There was a problem hiding this comment.
I can't this is desired behavior and tested I guess because this test fails https://github.com/DASPRiD/container-interop-doctrine/blob/master/test/ConnectionFactoryTest.php#L40 but I agree with you @Ocramius this should fail, but I guess this needs to be changed in more places so maybe leave it for next time or remove the test case 😄
src/ConnectionFactory.php
Outdated
| Type::addType($name, $className); | ||
| } | ||
|
|
||
| self::$areTypesRegistered = true; |
There was a problem hiding this comment.
Flag should be moved up, or else the logic may be re-executed if any of the Type::* operation throws an exception
There was a problem hiding this comment.
it would override type but not throw exception but ok does not make much difference
There was a problem hiding this comment.
I read one more time and now I got what you say 😄 should be ok now
|
Merging as failure came from HHVM and we don't care about that any more. Gonna remove that in another PR. |
|
I rebased this as wasn't keen on the merge commit that the GitHub UI's conflict resolution created. This is now merged in a4833d2. |
This allows creating connection without entitymanager and still have working types.
Before this fix creating connection throwed exception: