[DoctrineBridge] Require class option for DoctrineType

[jmikola]

I was updating the DocumentType class in the MongoDB bundle and realized that the class attribute was required but not enforced by OptionsResolver.

It's been a while since I used this field, but perhaps other options should be marked as required, too. Please let me know.

/cc @beberlei @bschussek

[jmikola]

Without the class option provided, a null class value gets passed to the object manager's getClassMetadata() method, which results in a PHP error in class_parents():

1) Doctrine\Bundle\MongoDBBundle\Tests\Form\Type\DocumentTypeTest::testDocumentManagerIsReturnedWhenGettingEm
class_parents(): object or string expected

/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/RuntimeReflectionService.php:40
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:257
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:281
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:205
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/DocumentManager.php:292
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php:102
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php:121
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/OptionsResolver/Options.php:466
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/OptionsResolver/Options.php:308
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/OptionsResolver/OptionsResolver.php:231
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/Form/ResolvedFormType.php:116
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/Form/FormFactory.php:99
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/Form/FormFactory.php:50
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/Tests/Form/Type/DocumentTypeTest.php:41

Additionally, omitting the em option leads to another PHP error as null is passed to spl_object_hash():

1) Doctrine\Bundle\MongoDBBundle\Tests\Form\Type\DocumentTypeTest::testDocumentManagerIsReturnedWhenGettingEm
spl_object_hash() expects parameter 1 to be object, null given

/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php:103
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/OptionsResolver/Options.php:466
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/OptionsResolver/Options.php:308
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/OptionsResolver/OptionsResolver.php:231
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/Form/ResolvedFormType.php:116
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/Form/FormFactory.php:99
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/vendor/symfony/symfony/src/Symfony/Component/Form/FormFactory.php:50
/home/jmikola/workspace/doctrine/DoctrineMongoDBBundle/Tests/Form/Type/DocumentTypeTest.php:41

However, I'm hesitant to require the em option, as non-ORM types extending DoctrineType may use their own option (e.g. document_manager) and set the em option internally. Requiring em at the OptionsResolver level seems to throw an exception before we can copy the option.

[jmikola]

Looking a bit more into the em option and the spl_object_hash() error, it looks like the $choiceList option Closure is executing before the em option's normalizer Closure, which should set an empty em option by running the class option through getManagerForClass(). I'd reckon that's a bug, since em need not be provided.

[travisbot]

This pull request passes (merged d96da28b into d3625b0).

[jmikola]

2cc7d2f44f4c1252dd2ac9964f0f92dce96f051b should be a failing test case at the moment.

[travisbot]

This pull request fails (merged 2cc7d2f4 into d3625b0).

[stof]

em is not required as it will guess it otherwise (searching the manager supporting the class). However, the handling of invalid classes (not mapped to a manager) may be missing (the case where getManagerForClass() returns null)

[jmikola]

The em option definitely should not be required, but the test case above (for inferring it based on class) fails because of Options::all():

public function all()
{
    $this->reading = true;

    // Performance-wise this is slightly better than
    // while (null !== $option = key($this->lazy))
    foreach ($this->lazy as $option => $closures) {
        // Double check, in case the option has already been resolved
        // by cascade in the previous cycles
        if (isset($this->lazy[$option])) {
            $this->resolve($option);
        }
    }

    foreach ($this->normalizers as $option => $normalizer) {
        if (isset($this->normalizers[$option])) {
            $this->normalize($option);
        }
    }

    return $this->options;
}

em is only inferred from class by its normalizer, but normalization happens after resolution and the resolving closure for choice_list depends on em being an object.

[stof]

@bschussek this looks weird

[jmikola]

One possible solution would be to duplicate the em normalization within choice_list's closure in order to compute the SPL object hash. Otherwise, I imagine the choice_list logic would have to take place in a normalizer; however, that would be quite fragile, as it'd depend on em appearing first in the normalizer array.

[stof]

but this seems weird. The entity type does not have such issue, and the em is also using a normalizer in the parent DoctrineType

[jmikola]

I think EntityType is affected by this. See 2cc7d2f44f4c1252dd2ac9964f0f92dce96f051b

[fabpot]

What's the status of this PR?

[jmikola]

We were waiting for @bschussek to follow-up. I'll try to sit with him today if he arrives before I leave to catch my plane.

[jmikola]

Resubmitting against 2.1 in #5632.