Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with sonata.media.manager.media service and doctrine_phpcr db_driver #509

Closed
lucasgranberg opened this issue Mar 13, 2014 · 37 comments

Comments

@lucasgranberg
Copy link

I dont know if this is becouse of changes in #494 but I get

Argument 2 passed to Sonata\\CoreBundle\\Model\\BaseManager::__construct()
must implement interface Doctrine\\Common\\Persistence\\ManagerRegistry,
instance of Sonata\\DoctrinePHPCRAdminBundle\\Model\\ModelManager given

when I try to use db_driver: doctrine_phpcr for sonata_media.

If I modify the sonata.media.manager.media like this:

<service id="sonata.media.manager.media" class="%sonata.media.manager.media.class%">
            <argument type="service" id="sonata.media.pool" />
            <argument type="service" id="doctrine_phpcr" />
            <argument>%sonata.media.media.class%</argument>
        </service>

(using doctrine_phpcr service instead of sonata.admin.manager.doctrine_phpcr)
It get past that problem but then I get

The class 'Application\Sonata\MediaBundle\Entity\Gallery' 
was not found in the chain configured namespaces

Why is it searching for an entity when I use doctrine_phpcr?

@matheo
Copy link
Contributor

matheo commented Mar 13, 2014

In your config.yml, do you have doctrine.orm.auto_mapping: true? or a
"manual" list of Bundles to be mapped, with SonataMediaBundle missing in
that list?

@lucasgranberg
Copy link
Author

I am rocking it manual and I have both SonataMediaBundle and ApplicationSonataMediaBundle.

    orm:
        entity_managers:
            default:
                auto_mapping: false
                mappings:
                    FOSUserBundle: ~
                    ApplicationSonataUserBundle: ~
                    SonataUserBundle: ~
                    SonataMediaBundle: ~
                    ApplicationSonataMediaBundle: ~

@lucasgranberg
Copy link
Author

I can't get doctrine_phpcr to work as db_driver for sonata media. Does anyone have working code on github that I can take a look at?

@dantleech
Copy link
Contributor

@rande what needs to be done to get PHPCR working? If you point me in the right direction I will make a PR soon :) Are the database specific bundles deprecated now? I see alot of DB specific stuff in the CoreBundle.

@lucasgranberg
Copy link
Author

Is it possible to reference RDBMS entities from PHPCR instead? What are the benefits of having the media in PHPCR? I am just looking for alternatives and trying to figure out what is best.

@dantleech
Copy link
Contributor

I think the advantages are that they are referencable within PHPCR and also that you can have access to images at specific paths, like in a filesystem, e.g. /cms/images/site-logo.

You could reference the ORM medias from PHPCR, but it wouldn't be a formal reference, and you would have the mutable ID vs. immutable path problem while developing I guess.

But I too am wanting to use the MediaBundle in a project atm, but havn't managed to get this working yet. @lsmith77 do you think sonata-project/SonataDoctrinePhpcrAdminBundle#246 will help here?

@lucasgranberg
Copy link
Author

Okay, now I see why it is better.
I tried to get phpcr working on my own but I get confused by all the different files. We have Document and PHPCR folders in the mediabundle. Why are both needed? What are the differences? Is one for mongoDb or something?

@lucasgranberg
Copy link
Author

@dantleech Any progress? I have put the better part of this day trying to get it to work. I have made some progress and I can now create new medias. It is very hackish and I did it mostly to figure out what was missing.

First I did as I said in the first comment of this issue. At that point I tought it was just a bug but it seems like a lot of stuff in phpcr is not ready yet.
Is it a good idea to not use the service sonata.admin.manager.doctrine_phpcr? I should probably look in to that.

Where in the document tree is the media supposed to go? For now I modified my Media.phpcr.xml to this:

<document name="Application\Sonata\MediaBundle\Document\Media"
              repository-class="Application\Sonata\MediaBundle\Repository\MediaRepository" >

        <id name="id" type="id">
            <generator strategy="repository" />
        </id>
        <parentDocument name="parent" />

    </document>

Then I hardcoded the parent id value in the Sonata MediaAdmin. Should there be a option for selecting the basepath and should there be a setting for overrideing the admin class? I can create my own admin class but if I inject it I end up with two admins for the mediabundle.

I also had to add my own mediarepository. I am doing something wrong when I generate the ids as my images can't be shown and the paths go all wonky. I suspect that the 'sonata.admin.manager.doctrine_phpcr' service would have helped here instead of doctrine_phpcr. I havn't looked into what that service even does.

@lucasgranberg
Copy link
Author

Sorry for nagging but how is it going @dantleech ? Is there any hope for phpcr support for the sonatamediabundle? I tried myself but I wasn't really up for the task.

@lsmith77
Copy link

cc @rmsint

@dantleech
Copy link
Contributor

@rande ping .. any hints on what we need to do here?

@rmsint
Copy link
Contributor

rmsint commented Apr 17, 2014

Catching up a bit. I think the code for PHPCR is from 9 months ago, before the CmfMediaBundle was created. I don't understand the problem exactly, however I can imagine fe. that the manager used from the SonataDoctrinePhpcr...Bundle has changed and is out-of-sync.
...
Aha, reading further up, I see in the description of this issue that you figured out that the manager is out-of-sync.

@rmsint
Copy link
Contributor

rmsint commented Apr 17, 2014

Maybe this is related to handling different doctrine implementations? The phpcr code dates from before this was made possible: http://symfony.com/doc/current/cookbook/doctrine/mapping_model_classes.html

@lucasgranberg have you tried with a prefix?:

# app/config/config.yml
doctrine_phpcr:
    odm:
        auto_mapping: true
        mappings:
            SonataMediaBundle:
                prefix: Sonata\MediaBundle\PHPCR

See https://github.com/sonata-project/SonataMediaBundle/blame/master/Resources/doc/reference/installation.rst#L72.

Also I read in another comment about the repository you created, the parent id in there is correct. A bit more elegant could be to provide the basepath as a configurable parameter. Long time ago I played with an example in the cmf-sandbox, the PR is left there I think. ... Yep, see fe. here: https://github.com/symfony-cmf/cmf-sandbox/pull/159/files#diff-2cfc9d83080dfe5c3845b316404bbfdaR9

@dantleech as TODO imho at least the doctrine mapping compiler pass can be implemented instead of the prefix hack.

Another, larger TODO is that the CmfMediaBundle interfaces should be implemented for the PHPCR model. Also the gaufrette adapter prepared in the CmfMediaBundle can be used to use PHPCR as filesystem in the providers.

A challenge is how the provider creates new documents. For Phpcr the document that represents a media also is the filesystem that stores the binary content. See https://github.com/sonata-project/SonataMediaBundle/blob/master/Provider/FileProvider.php#L296, there you can see that the Media::$binaryContent parameter is used to store an uploaded file and then written to a (gaufrette) filesystem.

And probably also challenging is how to handle hierarchical data, there is not really an implementation for that in the SonataMediaBundle as far as I know.

@lucasgranberg
Copy link
Author

Yes, I used the path prefix. I tried different combinations out of pure desperation. I had extended sonatamediabundle with easyextend. Am I supposed to map SonataMediaBundle or ApplicationSonataMediaBundle? I tried with these mappings:

            SonataMediaBundle:
                prefix: Sonata\MediaBundle\PHPCR
            ApplicationSonataMediaBundle: 
                prefix: Application\Sonata\MediaBundle\PHPCR

I will have to try it all again today as I am not really shure what I have tried and what not.

@dantleech
Copy link
Contributor

I guess we should just write the PHPCR implementation from scratch, copying the existing ORM stuff rather than try and "fix" this. (?)

@rande
Copy link
Member

rande commented Apr 22, 2014

I forgot to migrate the GalleryManager for the PHPCR implementation in the commit: 68487e4#diff-275947b011df35e48ac10811b941a9d6

I guess, this is part of the problem, also, I misunderstood the Doctrine Registry usage as the PHPCR has its own Registry.

Also classes need to be correctly set, see configuration.
https://github.com/sonata-project/SonataMediaBundle/blob/master/DependencyInjection/Configuration.php#L390-L392

@lucasgranberg
Copy link
Author

I have my classes defined like this:

sonata_media:
    class:
        media: Application\Sonata\MediaBundle\PHPCR\Media
        gallery: Application\Sonata\MediaBundle\PHPCR\Gallery
        gallery_has_media: Application\Sonata\MediaBundle\PHPCR\GalleryHasMedia

I fixed the problem with the registry with the hack I mentioned in the first post. These are the problems I have found so far.

  • The skeleton for the mediabundle dictates that id generation should be done with a repository. Do we need to do it with a repository? In that case a basic skeleton for that should be added.
  • The Documents (Media and Gallery) has no parent or nodename.
  • If parent is added to the document it has to be set in the admin

@rmsint
Copy link
Contributor

rmsint commented Apr 23, 2014

I agree with @dantleech to start from scratch only then using the CmfMediaBundle interfaces and model then to try and "fix" the "old" phpcr code.

@lucasgranberg
Copy link
Author

I agree. I don't think I know how to do it so I will use cmfmediabundle for now. I might give it a try but I can't promise any results.

@rmsint
Copy link
Contributor

rmsint commented Apr 23, 2014

For me it is some time ago however i still have some ideas. If there is an interest in it I can try to put those ideas in a PR to have a start, then maybe more people can work on it also?

@benglass
Copy link

This seems to be broken in the most recent stable release to the point that its throwing an exception that breaks our entire application even though we are not using phpcr with sonata media. This is because the mappings appear to be invalid.

I'll investigate why this is happening but the tests aren't failing (perhaps because the tests don't test with PHPCR installed?) but assuming this is the case and considering that this functionality does not work until it gets refactored it seems like it should be removed from the stable release asap.

  [Doctrine\Common\Persistence\Mapping\MappingException]                                                                                         
  Invalid mapping file 'Sonata.MediaBundle.Document.BaseGalleryHasMedia.phpcr.xml' for class 'Sonata\MediaBundle\Document\BaseGalleryHasMedia'.  

@benglass
Copy link

@dbu @dantleech Trying to create a PR for this issue and its clear the mapping files are not correct but that is easily fixable (changing doctrine-phpcr-mapping to doctrine-mapping and a few other minor changes). The issue im running into now is that docrine odm seems to be automatically finding the classes in the Document namespace in this bundle and throws an exception because it cant find mapping files for them. Is this expected behavior for it to look in the Document namespace as well as the Doctrine\Phpcr namespace? Is there a way to disable it since those aren't actually PHPCR documents?

@benglass
Copy link

Looks to me like PHPCR will detect any classes in the Document folder and try to find mappings for them due to the extension extending this class

https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php#L174

and implementing this method which returns 'Document'

https://github.com/doctrine/DoctrinePHPCRBundle/blob/master/DependencyInjection/DoctrinePHPCRExtension.php#L465

Which causes the odm mappings to look in the Document folder as the prefix by default. This code is pretty deeply nested so I'm not 100% on my detective work here, hoping for confirmation from dantleech or dbu.

It is possible to store your documents in another directory and have them mapped using this approach used by the symfony cmf bundles https://github.com/symfony-cmf/MenuBundle/blob/master/CmfMenuBundle.php#L27

It seems to solution is to move the cmf related classes out of the document namespace. Unfortunately this seems to have the side effect of also requiring that NOTHING is in the Document namespace lest it be found by the phpcr mapping code and cause this exception because no mapping data can be found.

@dbu
Copy link

dbu commented Jun 18, 2014

is your question answered by my comment on doctrine/DoctrinePHPCRBundle#152 ? basically you have to disable auto mapping for phpcr-odm if you have things in Document that are mapped for something else. and the cookbook article i linked over there explains how to use those compiler passes as we do in the cmf bundles.

in a end project, you can simply configure the folders for each type of documents instead of having auto mapping or needing the compiler passes.

@benglass
Copy link

Yes that was helpful. That is what i suspected. In this case the issue is actually that the classes in the Document namespace should are NOT phpcr odm classes so they dont have mappings at all which causes an exception. The solution seems to be as you noted to disable auto mapping. In this bundle its unfortunate that users who happen to have PHPCR ODM bundle installed but do not use it for sonata media would be forced to switch off auto mapping to avoid these Document classes causing this exception. The only alternative would be to move these classes out of the Document namespace in this bundle which Im going to explore (not sure they are even used for anything at the moment but they might be used by mongo)

@benglass
Copy link

@lucasgranberg @dantleech it sounds like you got this working lucas but to summarize

  1. I am submitting a PR to fix the PHPCR mappings for this bundle because they are broken/outdated
  2. If you have PHPCR ODM Bundle installed and you are using this bundle then you MUST disable auto mapping because PHPCR ODM Bundle will try to find mapping files for anything it finds in the Document folder. In this bundle those are Mongo documents so it throws an exception due to lack of mapping. I think you need mapping like the following or to disable the mappings completely for this bundle and provide your own.
    orm:
        entity_managers:
            default:
                auto_mapping: false
                mappings:
                    SonataMediaBundle:
                        mapping:              true
                        prefix:                   Sonata\MediaBundle\PHPCR

@rande
Copy link
Member

rande commented Jun 18, 2014

@benglass GG

@benglass
Copy link

This is basically resolved but could probably used some documentation since use of media bundle with phpcr bundle installed requires users to disable auto mapping

@benglass
Copy link

On further exploration its possible this could work without requiring users to disable auto mapping. I dug further into the problem and the issue is outlined here doctrine/DoctrinePHPCRBundle#152

Essentially both PHPCR and MongoDB use 'Document' as the default namespace. The fact that the mappings for PHPCR are in Resources/config/doctrine and match the names of the files in the Document namespace but dont provide mappings for them (they are for the files in the PHPCR namespace) causes this mapping exception. I believe the fix is to move the phpcr mappings to Resources/config/doctrine-phpcr and associate them with the PHPCR directory via an explicit call to https://github.com/doctrine/DoctrinePHPCRBundle/blob/master/DependencyInjection/Compiler/DoctrinePhpcrMappingsPass.php#L70 which is the approach used by the symfony cmf bundles and is the recommended approach for developing re-usable bundles.

Example https://github.com/symfony-cmf/MenuBundle/blob/master/CmfMenuBundle.php#L26

I will submit a PR for this change as it shoudl enable used to use auto mapping or not.

@dantleech
Copy link
Contributor

@benglass have you managed to get this bundle working with PHPCR?

@benglass
Copy link

@dantleech no I am not using PHPCR as a driver for this bundle so I was primarily concerned with making it so the fact that PHPCR code existed and that I had sonata phpcr admin installed didnt cause sonata media bundle to explode. This mainly involved fixing the mapping files (there was some outdated xml schema issues) and disabling auto mapping.

Right now my PR above is failing tests because of an issue with PHPUnit and BC PHP change that causes the error

Failed asserting that exception message 'Internal classes cannot be mocked without invoking their constructor in PHP 5.4.29' contains 'Unable to flush : Failed!!'.

This is unrelated to the changes I made and has to do with mocking internal PHP Classes. There is more info here #589

@dantleech
Copy link
Contributor

ok, I have had another go at this, after fixing lots of minor problems with generation and configuration I got as far as the content being registered in the PHPCR repository: https://gist.github.com/dantleech/1e16a1d384462b2d28ff

But am now getting this exception:

An exception has been thrown during the rendering of a template ("Interface Doctrine\ORM\Proxy\Proxy does not exist") in SonataMediaBundle:MediaAdmin:edit.html.twig at line 73.

Will try and look into it again soon and make some PRs

@dantleech
Copy link
Contributor

I have started a PR to address this issue: sonata-project/SonataCoreBundle#82

@rande rande mentioned this issue Jul 28, 2014
7 tasks
@dantleech
Copy link
Contributor

This will be fixed in #602 alright to close this @lucasgranberg ?

@rande
Copy link
Member

rande commented Jul 30, 2014

@lucasgranberg closing for now, if you thing it is not valid just ping the issue

@rande rande closed this as completed Jul 30, 2014
@lucasgranberg
Copy link
Author

Sorry for no answere. Came back from vacation recently.
Its ok close as far as I am concerned.

@kakoma
Copy link

kakoma commented Dec 9, 2015

Tried variations of all the suggestions here without much headway(including using the updated documentation https://sonata-project.org/bundles/media/master/doc/reference/installation.html); .
Might re-attempt this later but for now,switched to doctrine_orm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants