[Fixture] Strange DocumentManager argument... typo?

[cleentfaar]

Hey guys,

Was getting started on working with the CMF today and got around till the 2nd chapter of the guide, to find a strange typo in a code snippet here.

The argument is named $documentManager, the class DocumentManager gets imported, but ObjectManager is actually used as the type hint...?

There are some 10+ similar cases of these in the documentation, which made me think if this perhaps is not a typo but just a wrong interpretation on my hand; that ObjectManager should have been imported instead?

I then wondered if a similar thing is happening in the DemoBundle, and it is:

namespace Acme\DemoBundle\DataFixtures\PHPCR;

use Doctrine\ODM\PHPCR\DocumentManager;
// ...
use Doctrine\Common\Persistence\ObjectManager;
// ...

/**
 * Loads the initial demo data of the demo website.
 */
class LoadDemoData implements FixtureInterface
{
    /**
     * Load data fixtures with the passed DocumentManager
     *
     * @param DocumentManager $manager
     */
    public function load(ObjectManager $manager)
         // ... only ObjectManager used here
    }
}

The PHPDoc tells me it's a DocumentManager, the argument says ObjectManager, both get imported, but only the ObjectManager is actually used in the rest of the class.

I was making a PR of the documentation cases, thinking it was only a documentation error. But I realized more than just the documentation is affected; so perhaps this is all just a misunderstanding on my end, and some things just need some more clarification.

If any of you want to give me some pointers on how all of this project's load() examples should be named and imported consistently (i.e. as you imagine it), including that of AcmeDemoBundle's, I'm happy to make a larger PR for that as well.

[dbu]

the fixtures build on the common doctrine fixtures bundle. this bundle only knows about the abstract ObjectManager. the class is defined in the fixtures interface so we can't use anything different.

for our fixtures, we know that we can expect a PHPCR-ODM DocumentManager. this is why we document it like this.

strictly speaking, we could do an instanceof check and throw an exception as first thing in the load method, if somebody would call the fixtures with the orm fixture loader. in our projects we never bothered, because we "knew what we are doing" :-) - which is not the right attitude for teaching how things work... if you want to do a PR to add an instanceof check, i think that would be correct and could help avoid confusion.

[wouterj]

Since it lives in the PHPCR namespace, it is only called with the DocumentManager of the ODM, isn't it?

[cleentfaar]

@wouterj Yes that's what I thought; shouldn't it actually be the DocumentManager in those cases? But I'm happy to update this PR with some instanceof checks if that at least can help clarify how this code is used. Will be on it tonight.

[wouterj]

No, @cleentfaar. ObjectManager is the main manager class. Each implementation (ORM, PHPCR ODM, etc.) extends that class and implements the specific things for that implementation. That's where DocumentManager (for ODM) and EntityManager (for ORM) are comming from.

The DataFixture class is also implementation agnostic. This means that it should work with all doctrine implementations. That's why it type hints for ObjectManager and not a specific implementation.

You can't change the type hint, PHP will give an error for that. So you just use the ObjectManager typehint.

But @dbu said we need to use an instanceof check to be sure that it is a DoctrineManager. I don't think that's true, since the data fixture lives in DataFixtures/PHPCR, that means that it is only run when using the PHPCR-ODM implementation.

[dbu]

@wouterj if you just call doctrine:phcpr:fixtures:load, this is true. but you could also place your fixtures in app/Resources/Fixtures or something, and call doctrine:phcpr:fixtures:load app/Resources/Fixtures - if you give the path manually, the naming convention does not protect from mistakes. i could do doctrine:mongo:fixtures:load src/Acme/DemoBundle/DataFixtures/PHPCR (or however the mongo command is called).

[lsmith77]

so what needs to be done here? adding instanceof checks?

[dbu]

yeah, we should add instanceof checks to our fixtures in the standard-edition and the sandbox, as well as in the doc. and in the doc we could add a note why there is a mismatch between phpdoc and method signature.

[dbu]

@cleentfaar do you want to have a go at those PR?

[lsmith77]

ping

[lsmith77]

fixed in #18 and symfony-cmf/cmf-sandbox#264 and symfony-cmf/symfony-cmf-docs#596