Added image type #37

Merged
merged 10 commits into from Feb 27, 2013

Projects

None yet

7 participants

@elHornair

This PR adds an phpcr odm image type that can be used in forms to upload an image that is stored in a PHPCR repository.

@dbu dbu and 2 others commented on an outdated diff Feb 22, 2013
Document/Image.php
@@ -0,0 +1,61 @@
+<?php
+
+namespace Doctrine\Bundle\PHPCRBundle\Document;
@dbu
dbu Feb 22, 2013 Doctrine member

should we put the Image Document into phpcr-odm itself? (this is really a question, i am not sure if it makes much sense)

@elHornair
elHornair Feb 22, 2013

Hmm, I don't know either :)

Can we leave it here for now and move it when its needed somewhere else?

@dbu
dbu Feb 22, 2013 Doctrine member

moving would be a BC break so not ideal. any opinions, @lsmith77 ?

@lsmith77
lsmith77 Feb 22, 2013 Doctrine member

lets put it into the ODM since we already have a couple of documents there. i guess we can then also default to this inside CreateBundle?

@elHornair
elHornair Feb 22, 2013

Ok, will do. Yes, the one in CreateBundle looks pretty much the same

@lsmith77
lsmith77 Feb 22, 2013 Doctrine member

merged

@elHornair
elHornair Feb 23, 2013

Great, thank you. Now this PR is not considered WIP anymore. Or is there something left that should be adapted?

@dbu dbu and 1 other commented on an outdated diff Feb 22, 2013
Document/Image.php
+ /**
+ * @PHPCRODM\Id
+ */
+ protected $path;
+
+ /**
+ * Image file child
+ *
+ * @PHPCRODM\Child(name="file", cascade="persist")
+ */
+ protected $file;
+
+ /**
+ * @PHPCRODM\String
+ */
+ protected $mimeType;
@dbu
dbu Feb 22, 2013 Doctrine member

i think we should map the mimeType attribute of the jcr:content of the nt:file child

@dbu dbu and 1 other commented on an outdated diff Feb 22, 2013
Document/Image.php
+ protected $path;
+
+ /**
+ * Image file child
+ *
+ * @PHPCRODM\Child(name="file", cascade="persist")
+ */
+ protected $file;
+
+ /**
+ * @PHPCRODM\String
+ */
+ protected $mimeType;
+
+
+ public function setFile($file)
@dbu
dbu Feb 22, 2013 Doctrine member

can we typehint File here?

@elHornair elHornair referenced this pull request in doctrine/phpcr-odm Feb 22, 2013
Merged

added image document #250

@dbu dbu and 1 other commented on an outdated diff Feb 23, 2013
Resources/views/Form/fields.html.twig
@@ -0,0 +1,4 @@
+{% block phpcr_image_widget %}
+ {{ form_widget(form) }}
+ <img src="{{ form.vars.data | imagine_filter('image_upload_thumbnail') }}" alt="" />
@dbu
dbu Feb 23, 2013 Doctrine member

what happens with this widget if i have not yet uploaded an image?

and what is the standard way with the symfony Form to distinguish between "user did not upload a new file" and "user wants to remove that file"?

is this imagine filter using the copy of the image, so in the end file system to display it?

@elHornair
elHornair Feb 25, 2013

Uh yeah, if I have not yet uploaded an image, I still try to display it - will fix.

I don't know how we can distinguish between "no new file uploaded" and "file should be removed" - does someone know? Maybe add a checkbox to explicitly delete it?

Yes, imagine_filter uses a file system copy. Will try to add a listener to make sure we clear the imagine cache whenever a new image gets set

@dbu
Member
dbu commented Feb 23, 2013

how exactly does it work with showing the image? is the imagine filter fine for all? or do we need an image controller? should we port https://github.com/symfony-cmf/CreateBundle/blob/master/Controller/PHPCRImageController.php here? or should all go through imagine?

@dantleech dantleech commented on the diff Feb 23, 2013
Form/DataTransformer/ModelToFileTransformer.php
@@ -0,0 +1,38 @@
+<?php
+
+namespace Doctrine\Bundle\PHPCRBundle\Form\DataTransformer;
+
+use Symfony\Component\Form\DataTransformerInterface;
+use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Doctrine\ODM\PHPCR\Document\File as PHPCRFile;
+use Doctrine\Common\Util\Debug;
+use Doctrine\ODM\PHPCR\Document\Image;
+
+class ModelToFileTransformer implements DataTransformerInterface
+{
+
@dantleech
dantleech Feb 23, 2013 collaborator

No need for blank line here I think

@dantleech dantleech commented on an outdated diff Feb 23, 2013
Form/DataTransformer/ModelToFileTransformer.php
+
+use Symfony\Component\Form\DataTransformerInterface;
+use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Doctrine\ODM\PHPCR\Document\File as PHPCRFile;
+use Doctrine\Common\Util\Debug;
+use Doctrine\ODM\PHPCR\Document\Image;
+
+class ModelToFileTransformer implements DataTransformerInterface
+{
+
+ /**
+ * {@inheritdoc}
+ */
+ public function reverseTransform($uploadedFile)
+ {
+ if (! $uploadedFile instanceof UploadedFile) {
@dantleech
dantleech Feb 23, 2013 collaborator

I think this should be !$uploadedFile (i.e. no space between)

@dantleech dantleech and 1 other commented on an outdated diff Feb 23, 2013
Form/DataTransformer/ModelToFileTransformer.php
+use Doctrine\Common\Util\Debug;
+use Doctrine\ODM\PHPCR\Document\Image;
+
+class ModelToFileTransformer implements DataTransformerInterface
+{
+
+ /**
+ * {@inheritdoc}
+ */
+ public function reverseTransform($uploadedFile)
+ {
+ if (! $uploadedFile instanceof UploadedFile) {
+ return $uploadedFile;
+ }
+
+ /** @var $uploadedFile UploadedFile */
@dantleech
dantleech Feb 23, 2013 collaborator

What is the policy on in function type hints like this? @dbu ?

@dbu
dbu Feb 24, 2013 Doctrine member

i tend to do them, as it makes tools like phpstorm show you exactly if you do things right or try to call something that does not exist.

@dantleech dantleech commented on the diff Feb 23, 2013
Form/Type/ImageType.php
@@ -0,0 +1,28 @@
+<?php
+
+namespace Doctrine\Bundle\PHPCRBundle\Form\Type;
+
+use Symfony\Component\Form\AbstractType;
+use Symfony\Component\Form\FormBuilderInterface;
+use Doctrine\Bundle\PHPCRBundle\Form\DataTransformer\ModelToFileTransformer;
+
+class ImageType extends AbstractType
+{
+
@dantleech
dantleech Feb 23, 2013 collaborator

Unneeded blank line

@dantleech dantleech commented on the diff Feb 23, 2013
Form/Type/ImageType.php
+ public function getParent()
+ {
+ return 'file';
+ }
+
+ public function getName()
+ {
+ return 'phpcr_image';
+ }
+
+ public function buildForm(FormBuilderInterface $builder, array $options)
+ {
+ $transformer = new ModelToFileTransformer();
+ $builder->addModelTransformer($transformer);
+ }
+
@dantleech dantleech and 2 others commented on an outdated diff Feb 23, 2013
Form/Type/ImageType.php
+
+use Symfony\Component\Form\AbstractType;
+use Symfony\Component\Form\FormBuilderInterface;
+use Doctrine\Bundle\PHPCRBundle\Form\DataTransformer\ModelToFileTransformer;
+
+class ImageType extends AbstractType
+{
+
+ public function getParent()
+ {
+ return 'file';
+ }
+
+ public function getName()
+ {
+ return 'phpcr_image';
@dantleech
dantleech Feb 23, 2013 collaborator

Should we now be calling these things "phpcr_odm_*" ? @dbu

@dbu
dbu Feb 24, 2013 Doctrine member

+1 for phpcr_odm_image - this is talking about documents, not about phpcr nodes.

@dbu dbu referenced this pull request in symfony-cmf/cmf-sandbox Feb 24, 2013
Closed

[WIP] sonata media example #159

@rmsint
rmsint commented Feb 25, 2013

Wow, really nice. It would be usefull to also provide an option to delete the image. This can be used when an image is not required: the user uploaded one and then does not want the image anymore. Here is a gist from a project, could save some time if you want to add it: https://gist.github.com/rmsint/ad4d8804e3ae6fae2160

@dbu
Member
dbu commented Feb 25, 2013

ah cool. so you just add another field to the form to explicitly delete. sounds reasonable. when/where do you actually delete the image in your gist? can we access that in the data transformer?

the event listener sounds not straightforward for me. ah, or do we need it and can't make it in the custom type directly because we don't know at that point if we will have data or not?

@stof
Member
stof commented Feb 25, 2013

@dbu the type cannot handle data. It is about configuring the Form object, so it occurs before using the Form object. As you need to access the data to determine whether the field should be added, it can only be done later.

@elHornair

Nice @rmsint thx for that gist, will try to add this. However there are two things that are unclear to me:

  1. Where do you actually delete the image when the checkbox is checked? (what @dbu already asked)
  2. Did you also have the problem that in edit mode the image will be deleted (overridden by null) if you don't upload a new image? I don't understand how I could tell the form component "There is no new image, just keep the old".
@rmsint
rmsint commented Feb 25, 2013
  1. the checkbox is mapped to the setRemoveFile method on the object containing the image. I now realize this was not an image object but something like the slide object embedding the image, sorry... If the checkbox is checked it does not really makes sense to delete the file from the image object, an empty image object will then be referenced from fe. a slide object. And I think you then want the image object to be removed. Maybe you can still implement it only then have to document that the method removeFile (or something else) should be implemented on the parent of the image object? A bit the same as the collection type has the "allow_delete" option.
  2. no, but I followed most of the solution from the symfony cookbook for ORM: http://symfony.com/doc/current/cookbook/doctrine/file_uploads.html. The difference is that the file is stored on the filesystem and not in the document. Anyhow, it works this way:
    • an image object has a file property for uploads that is not persisted
    • then prePersist and preUpdate methods take care of handling a new file upload and will result in setting the path property of the image object
    • in edit mode when null is set to the file property, nothing will happen to the path property and the file on the filesystem
@dantleech
Collaborator

Is this type only for "updloaded" documents? What about documents that already exist, e.g. a user avatar? Should this be renamed to "phpcr_odm_image_upload" so that we can free up "phpcr_odm_image" to display, change and remove existing images?

@dbu
Member
dbu commented Feb 26, 2013

@dantleech well the type is to handle images in forms. which usually means uploading the image. i think the only other thing that could be would be providing the image by url and have the server grab it from there, but that is rather the special case. the Image document could be used for other contexts than the image form type we discuss here. but when would you need a phpcr_odm_image form type whithout uploading?

@rmsint thanks for the clarifications. so i guess we have to do the same in our Image document, not remove the file on null but have a removeFile method that can be triggered by the checkbox. @elHornair do you think you can manage that? guess we need to update the document in phpcr-odm as well.

@dantleech
Collaborator

Well, you might want to change the image reference to another existing image, for example another in a media library. In that case the media library would handle the upload.

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

David Buchmann notifications@github.com a écrit :

@dantleech well the type is to handle images in forms. which usually means uploading the image. i think the only other thing that could be would be providing the image by url and have the server grab it from there, but that is rather the special case. the Image document could be used for other contexts than the image form type we discuss here. but when would you need a phpcr_odm_image form type whithout uploading?

@rmsint thanks for the clarifications. so i guess we have to do the same in our Image document, not remove the file on null but have a removeFile method that can be triggered by the checkbox. @elHornair do you think you can manage that? guess we need to update the document in phpcr-odm as well.


Reply to this email directly or view it on GitHub.

@dbu
Member
dbu commented Feb 26, 2013

ok, i see. but that would then be a sonata_media_image or something, or not?

@dbu
Member
dbu commented Feb 26, 2013

for the documentation, this would belong into this section: http://symfony.com/doc/master/cmf/bundles/phpcr-odm.html#form-types

@dantleech
Collaborator

Hmm... the more I think about this the less sure I am that it should belong here, after all, there is nothing Image specific in the ODM. I would imagine that it might fit better into something like a symfony-cmf/MediaBundle. I think that this PR also adds a dependence on the imagine library also no?, which shouldn't be associated with DoctrinePHPCRODM in my opinion. What do you think? Sorry to be negative :(

@Burgov
Burgov commented Feb 27, 2013

imagine should probably be added to composer.json?

@dbu
Member
dbu commented Feb 27, 2013

@dantleech thanks for voicing concerns, better now than being sorry later. i think for the full scale media management, there is SonataMediaBundle which people are working on integrating with the cmf. this is just a small piece and it does bring the Image document of phpcr-odm into the form layer, so i think here is the right place. we create no hard dependency on imagine:

@burgov yes, alain is going to add a suggest entry for imagine.

if people do not want to use the form type, they do not need imagine, so its not creating a new hard dependency. we could even add an option to configure if you want preview - without preview there is no imagine needed at all.

@rmsint i will extract your gist and suggestion into a new ticket, atm we will just delete the whole document to get rid of the image (that really depends on the use case what makes sense, so your suggestion has value and should be implemented, but we can't do everything right now, as much as i would wish so...)

@elHornair

Did the cleanup. Thanks a lot for your explanations @rmsint

@dbu dbu merged commit 41f4939 into doctrine:master Feb 27, 2013
@dbu
Member
dbu commented Feb 27, 2013

i created #40 to keep the information from rmsint regarding the delete file checkbox. its a cool idea and if somebody has time would be a nice addition.

@dbu
Member
dbu commented Feb 27, 2013
@dantleech
Collaborator

@dbu I didn't realise the PHPCR-ODM had an Image class, so this PR makes sense to me now :)

@dbu dbu referenced this pull request in symfony-cmf/media-bundle Jun 21, 2013
Open

allow deletion in image form type #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment