adding media interfaces and a download controller draft

[dbu]

i slightly diverge from https://github.com/symfony-cmf/symfony-cmf/wiki/Media - sometimes to use the same names as https://github.com/sonata-project/SonataMediaBundle/blob/master/Model/MediaInterface.php and sometimes to make things simpler. lets discuss the details.

this should then replace the insecure code we have at https://github.com/liip/LiipImagineBundle/blob/master/Imagine/Data/Loader/DoctrinePHPCRLoader.php#L41 and we should move the model objects here from CreateBundle https://github.com/symfony-cmf/CreateBundle/blob/master/Document/Image.php and replace the interface https://github.com/symfony-cmf/CreateBundle/blob/master/Model/ImageInterface.php as well as the phpcr-odm image model class - having an image in phpcr-odm was the wrong place.

images should be output using imagine, but for downloads i think this controller makes sense. rather than allowing any download, we can have routes for downloadable documents, cleaner and at the same time simpler.

[rmsint]

a little detail.. caption tends to be image specific, maybe use a more generic name? sonata media uses "description" for the same purpose

[rmsint]

Awesome! What is the purpose of the download controller, is a simple action needed? It looks like duplicating some Sonata Media logic (see https://github.com/sonata-project/SonataMediaBundle/blob/master/Provider/FileProvider.php#L313), this one also includes "X-SendFile" protection. Not sure if you want to have that functionality in the CMFMediaBundle, I can imagine it is very usefull.

[dbu]

renamed the caption to description.

regarding the controller, i think it makes a lot of sense to have something generic to download from phpcr. thomas showed me the code in the sonata media bundle, it indeed is much more complex. we looked at the BinaryFileResponse a moment, and it does more than the controller of the sonata media bundle. ideally we would leverage the BinaryFileResponse of symfony. i still think it would make sense to have such a controller here. for outputting images, we rely on imagine but we should offer something simple to download files.

[rande]

there is some duplicate code here ;)

The BinarayFileResponse is new to sf2.2 and yes the download controller of SonataMediaBundle should be refactored to use this response object.

[dbu]

i know about the code duplication. the motivation was to have some minimal interfaces for media in the cmf so that other bundles that care about media can use this. depending on sonata media seemed like too many advanced features and things.

but if we move sonata media into the cmf organization, we will have to rethink this fundamentally.

my goal would be to have straightforward interfaces for media files that projects can use if they have simple needs. and keep the full complexity of the current sonata media bundle separate, for when a powerful solution is needed.

[rmsint]

Checked the interfaces once more and they look OK. I am only not sure about the getBinaryContent result. For SonataMedia a \SplFileInfo instance (or Symfony\Component\HttpFoundation\File or Symfony\Component\HttpFoundation\File\UploadedFile) is used. This looks not to be compatible with the Doctrine\ODM\PHPCR\Document\Resource, and this is used for LiipImagine isn't it? Do you have an idea how we could map the PHPCR file and SonataMedia to the same interface for the binary content?

[dbu]

no expert in php and binaries. the controler code is definitely not good yet. the problem is that we are not file based, so anything that wants a real file is broken for us. we need streams - but maybe SplFileInfo can handle streams, really don't know.
if we find something better here, that would be cool. maybe we have to ask on the general symfony channels if there is already something to work with streams rather than file handles...

[rmsint]

Did read some theory by now. And would like to check this in the mailing list later, I hope someone can help us further.

Streams seem to be more low-level in php and have the purpose to unify the methods for working on files, sockets and similar resources. I think we have to create a stream wrapper for phpcr and feed that to \SplFileInfo. Something like this will happen then:

$binaryContent = new \SplFileInfo('phpcr://path/to/file.ext');

Streams actually are used in the internals of fopen(), file_get_contents() and file(). Will have to check more, but I looks like the direction to tie everything together.

Drupal defined a StreamWrapperInterface so will look there. And Gaufrette also has the option to define StreamWrappers, but cannot find the relation with the File adapters, will check it too. I also found that the cloud provider HPCloud defined a stream wrapper to communicate with its cloud, learned from there that we could use the stream context for doing authentication stuff if needed.

The BinaryFileResponse of Symfony takes an \SplFileInfo as input . Once we have the getBinaryContent sorted out I think we have a better understanding what needs to be done for the download action in the controller.

[dbu]

note that jackalope already is using a stream wrapper
https://github.com/jackalope/jackalope/blob/master/src/Jackalope/BinaryStreamWrapper.php

depending on what we find out, we might move that to phpcr-utils and
make the odm wrap binary in case the phpcr impl is not jackalope? if the
wrapper is the thing we need...

[rmsint]

ok, here is a sum up of a chat with rande on #symfony-cmf:

• create a Gaufrette adapter for jackalope / phpcr (integration with SonataMedia is achieved by telling the providers to use the adapter, side effect is that we directly can setup a stream_wrapper with Gaufrette)
• the getBinaryResponse can be removed from the FileInterface
• the logic to handle storing data is to be done in a provider, so we can introduce something like FileProviderInterface with the method getReferenceFile that returns a Gaufrette\File, this allows a generic method to extract the binary content that also can be used with LiipImagine (and is compatible with the SonataMedia providers)

[dbu]

sounds like a plan. just to be sure i get this right:

can introduce something like |FileProviderInterface| with the method
|getReferenceFile| that returns a |Gaufrette\File|, this allows a
generic method to extract the binary content that also can be used
with LiipImagine (and is compatible with the SonataMedia providers)

what exactly do you mean by extract? not copying to the file system,
right? there is a phpcr provider for LiipImagineBundle that can directly
pipe the stream into imagine... would we rewrite that to use the same
gaufrette adapter? is LiipImagineBundle already using gaufrette?

[rmsint]

No, liipimagine does not have a dependency on gaufrette currently, checked it this evening. It think we should go more low-level and define a FileStreamProvider with getStream or something. Should we limit the phpcr provider of the LiipImagineBundle to only support the CmfMedia ImageInterface?

Currently I am looking at how the Phpcr odm file, a CmfMedia FileInterface and a CmfMedia ImageInterface could be supported by the same phpcr gaufrette adapter

[dbu]

well CmfMedia ImageInterface will extend the CmfMedia FileInterface. gaufrette is not about images but files in general, so do we need to distinguish that case?

for phpcr-odm file its more tricky. having phpcr-odm depend on CmfMedia sounds wrong. maybe we could make the class for nt:file configurable in phpcr-odm and have the phpcr-bundle suggest CmfMedia and autodetect it to use a model class provided by phpcr-bundle that extends phpcr-odm File to implement the interface?

regarding the LiipImagineBundle: will gaufrette help here? if not i think its sane to say this only works with CmfMedia. we currently do a bad case of duck typing (just assuming a method is there and does what we expect it to do). but maybe @lsmith77 has a different opinion on this - would love to hear his input.
we should not create a hard dependency, but just suggest and document that for the phpcr loader you do need that dep.

[rmsint]

Regarding phpcr-odm file: I agree with that way to go.

Regarding LiipImagineBundle: gaufrette will help here to make it Phpcr compatible and will add extra dependencies to LiipImagine when using the Phpcr dataloader. Although we are only interested in images and that we only know because of the CmfMedia ImageInterface (will add CmfMedia dependencies). I think we only should use Gaufrette for more advanced solutions (SonataMedia) and make it a suggest on CmfMedia.

Additionally I think atm we should aim to keep the CmfMedia interfaces general and applicable to both ORM and PHPCR. To achieve this we also need something like a EmbeddedInterface and FileSystemInterface to know how to extract the binary data in a provider. The PHPCR implementation of it can then be put in the CmfMedia bundle. For a LiipImagine dataloader it has the advantage that it can be a CmfMediaLoader that can handle both ORM and PHPCR files as long as they follow the CmfMedia interfaces.

I am not sure yet about the Gaufrette adapter, and I have to investigate more to know if it will have requirements for the CmfMedia interfaces. Difficulty there is how to handle the creation of the embedded file of an image, we must ensure the image is created before, otherwise the document is of the wrong type (if by default the path is created as Phpcr odm Folder). Also not sure for the adapter if we need (and if it is possible) to make it support Phpcr odm Files and Folders without CmfMedia interfaces. Maybe again make the CmfMedia a suggest to get more features.

Btw. what is the relation between the jackalope:// stream wrapper and the Phpcr odm "nt:resource"? (Or who can explain this?) To prevent we run into circles with streams and resources because with a gaufrette adapter a stream wrapper can also be created.

Sorry for the long comment, there are lots of design decisions to make.

[lsmith77]

@dbu: I will be in FR tomorrow .. can you explain me the topic in person?

[dbu]

@lsmith77 sure

[dbu]

@rmsint no worries about the lenght, glad you are looking into this!

an nt:file node usually has an nt:resource child. that is the phpcr level of identifying something as file content and defining which properties must exist for such a node. the jackalope:// stream wrapper is to handle any binary property (any node can have a property of type Binary, not only nt:file > nt:resource).

probably you are right that we should explicitly distinguish between model classes providing a stream and those providing a file link. for orm one would probably want to use https://github.com/l3pp4rd/DoctrineExtensions/blob/master/doc/uploadable.md which stores file system paths in the db. if somebody really hates putting files into phpcr (or has a special use case where the documents are managed by something else) even a phpcr document could provide a file system path.

yeah, CmfMedia should have as little deps as ever possible so it can be installed anywhere. providing adapters i.e. for gaufrette is good but should be optional.

[dbu]

ups, forgot to comment: discussed with @lsmith77 and we realized we solved it differently until now. the Image document has a child of type File and proxies the get resource method of the File. that way we can implement the interface in an Image document regardles of actual storage. without doing custom node types its also the only way to have additional information about an image. the document is here: https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/Document/Image.php - and we should move it out of there and into the CmfMediaBundle - having it in phpcr-odm is the wrong place. also the form things should move here imo doctrine/DoctrinePHPCRBundle#45

[rmsint]

fyi, (almost) completed research, I will try to add some documentation to the wiki and update the interfaces here. We can also start with the gaufrette adapter and the intergration with SonataMedia, at least to see that it works. Probably we also need to add a setter to the FileInterface.

@dbu, @lsmith77 is it an option to rename Doctrine\ODM\PHPCR\Document\File::getContent to Doctrine\ODM\PHPCR\Document\File::getResource (and also for the setter)? We can then add getContent() string and maybe setContent(content string) to the FileInterface if that is needed. I think this can help to make the implementation with the LiipImagine dataloader and the gaufrette adapter easier.

[dbu]

why do you want to rename? i agree that the naming is not
perfect-perfect but all methods are there:

setFileContent(string|stream)
stream getFileContentAsStream()
string getFileContent()

setContent and getContent are used for the Resource child - that is the
logic thing as the field is called content and is a Resource object.

is it a technical problem to have the getContent/setContent methods like
this? if so, we might really need to change, but it will be a painful
change as the methods are used in some places and if the method goes not
away but changes its semantics, its damn hard to debug. (for the cmf
parts that use it, most need to be refactored for media anyways, so that
is less an issue. more thinking about users of phpcr-odm doing custom
stuff - maybe there are not that many such users)

[rmsint]

To keep the method name short. A class implementing the FileInterface with a method getFileContent looks double to me from interface point of view. It could already be clear that when you want the content and know the class is a file, because the interface tells so, the method is getContent. It is also no big problem to use a different name. I am also OK with getFileContent() string, if we need it in the interface.

[dbu]

what if we keep getContent for whatever the implementation does, but have the interface define getContentAsString and getContentAsStream? and the same with setContentFromX? that would be very explicit and avoid collisions with getContent/setContent like the one we would have on the phpcr-odm File.

(while we could still BC break phpcr-odm File if really needed, other libs wanting to implement the interfaces probably just can't and then we would have a problem with the interface. the explicit names should not lead to clashes - if they already exist they really should have the right semantics already.)

[rmsint]

sounds great, it is very explicit this way and prevents collisions

[dbu]

considering that time to cmf 1.0 is running short, i think we should find the very minimum that makes sense and merge. it of course is useful to sanity check if the interfaces make sense, but i think we really need to merge a solution by the end of the hackday on 21.

[rmsint]

Yes absolutely, my goal is to be ready for a merge monday 17 and ultimately the 21th then.

[rmsint]

The models almost work now:

• tested this in the sandbox with the SlideshowBlock
• fix upload - the stream is empty when converting it from a file in Symfony\Cmf\Bundle\MediaBundle\Form\DataTransformer\ModelToImageTransformer.
• mixins need to be defined
• model locations should be corrected like in move document to model routing-bundle#112

Shouldn't we also define setters for most getters in the interfaces?

What is the preferred way to move forward? Split the code from #5 in separate PRs. So interfaces are updated here. One for models + stuff moved from PHPCR-ODM, one for each adapter if we want to keep them in CmfMedia. The Gaufrette adapter I would postpone then.

[dbu]

for the PR: not too complicated. though having the code moved from phpcr-odm separate will help with reviewing. but atm nobody is using this bundle yet, so basically we can just push everything in, then adjust the sandbox and see it all working, and still freely adjust the interfaces this week.

[dbu]

i would not define setters in the same interfaces. the interfaces should be minimal, and most places do not need to write, but just read. writing is only in sonata admin and there we can trust people to only provide classes with setters.

[dbu]

see #5