[1.0] Rework data loaders. Introduce mime type guesser. #291

makasim · 2013-12-23T14:55:10Z

I did a small research about formats problems (discussed here #288 (comment)).

So right now I think that data loader MUST return original image binary. Later in the controller we can wrap it with Image instance but still we have ability to guess mime type based on the content. The guessed format will be passed to Resolver store method.

This way we do not need any image objects at all

UPDATE

Changes done in this PR:

Introduced MimeTypeGuesserInterface and its simple implementation SimpleMimeTypeGuesser.
LoaderInterface:find returns string - binary content of the image (In future we can allow to return RawImage instance)).
DataManager:find always returns RawImage instance. The plan is to reuse this RawImage in other places like filter manager and cache resolver.
Remove dependency on imagine service in all loaders.

makasim · 2013-12-23T20:25:20Z

@havvg I've updated this PR a bit.

Introduced MimeTypeGuesserInterface and its simple implementation SimpleMimeTypeGuesser.
LoaderInterface:find returns string - binary content of the image (In future we can allow to return RawImage instance)).
DataManager:find always returns RawImage instance. The plan is to reuse this RawImage in other places like filter manager and cache resolver.
Remove dependency on imagine service in all loaders.

makasim · 2013-12-23T20:26:28Z

Controller/ImagineController.php

-     *
+     * @var ImagineInterface
+     */
+    protected $imagine;


@havvg this added temporally and will be removed in next PRs. The purpose is to keep this PR small and focused on one thing.

makasim · 2013-12-23T20:31:17Z

@ASKozienko please review

ASKozienko · 2013-12-24T14:02:51Z

Imagine/Data/DataManager.php

     */
    public function find($filter, $path)
    {
        $loader = $this->getLoader($filter);

-        return $loader->find($path);
+        $content = $loader->find($path);
+        $mimeType = $this->mimeTypeGuesser->guess($content);


throw something like UnsuportedContentTypeException when content is not an image

yes would say, more we have to throw exception if we failed to guess mime type. and also if the mime type is not an image.

not sure about custom exception, I think generic like LogicException would be enough.

makasim · 2013-12-25T13:31:19Z

@havvg squased&rebased. Ready for review.

makasim · 2013-12-27T11:20:21Z

@havvg I see you are back. dont forget to look at this PR.

havvg · 2013-12-27T12:45:33Z

Yeah, I'm kinda busy these days. I will take an insight look into all of your PRs on this weekend and will have the model stuff ready until then, too, so we can take a look on how to combine everything.

makasim · 2013-12-27T13:30:49Z

@havvg if you haven't done much yet, please look at this PR first. It already contains a model (I call it RawImage). I think it should solve all the problems and bottle neck we have now.

makasim · 2014-01-02T07:48:40Z

@havvg ping

havvg · 2014-01-03T09:08:28Z

Looks good to me. However I don't like the name RawImage, it's not necessarily "raw" (in terms of image data), so I would remove the prefix, and just name it Image.

havvg · 2014-01-03T09:10:00Z

Imagine/Data/DataManager.php

     */
    public function find($filter, $path)
    {
        $loader = $this->getLoader($filter);

-        return $loader->find($path);
+        $rawImage = $loader->find($path);
+        if (false == $rawImage instanceof RawImage) {


if (!$rawImage instanceof RawImage) {

makasim · 2014-01-03T09:17:47Z

Looks good to me. However I don't like the name RawImage, it's not necessarily "raw" (in terms of image data), so I would remove the prefix, and just name it Image.

@havvg Image may be confusing. Someone caт mix it up with image from Imagine library. For example we can have a method that takes the bundle one image public function applyFilters(Image $image); without checking the imported namespace I cannot say it is ours or the lib ones. RawImage make this situation clear.

havvg · 2014-01-03T09:19:58Z

Imagine/SimpleMimeTypeGuesser.php

+        try {
+            file_put_contents($tmpFile, $binary);
+
+            $mimeType = MimeTypeGuesser::getInstance()->guess($tmpFile);


Please inject a Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface instead of using the implementation.

@havvg I dont think symfony provide this MimeTypeGuesser as a service. If so should I create this service in liip image space?

havvg · 2014-01-03T09:40:31Z

Image may be confusing. Someone caт mix it up with image from Imagine library. For example we can have a method that takes the bundle one image public function applyFilters(Image $image); without checking the imported namespace I cannot say it is ours or the lib ones. RawImage make this situation clear.

I know what you mean, however you can't mix it up in this case, as we have to use ImageInterface when referring to imagines images. While thinking about it, we should also introduce the model as an interface and rely upon it, so the Data\LoaderInterface may return a different implementation (e.g. retrieved by an ORM).

What do you think about a Liip\ImagineBundle\Model namespace containing the ImageInterface and an Image (the current RawImage) implementing the interface? I know the names are still clashing, but that's actually the point of namespaces :-)

makasim · 2014-01-03T09:48:18Z

we should also introduce the model as an interface and rely upon it, so the Data\LoaderInterface may return a different implementation (e.g. retrieved by an ORM)

@havvg it is indeed a good idea.

What do you think about a Liip\ImagineBundle\Model namespace

👍 too

makasim · 2014-01-03T09:48:35Z

what about Binarry model name? because it could be not only image. it is just a binary data with calculated format\mimetype

havvg · 2014-01-03T09:55:22Z

Binary is fine by me. I just thought about the name and my first impression was like "What else but images?" .. and well, actually, we are only offering some system of caching filtered data (e.g. a thumbnail of an image). But there is no limit to have it be a video which is resampled, for example. Well, there currently is a limitation in the dependency of imagine, but that's to be addressed in an other PR :)

makasim · 2014-01-03T17:42:54Z

@havvg updated, here's the list of changes I did after your last review:

Create services for symfony mime_type_guesser and extension guesser
Move SimpleMimeTypeGuesser and its itnerface to Binary namespace. It was in Imagine one and I think it is not the right place in any case.
Move Guesser by content to Binary namespace. It was in Imagine one and I think it is not the right place in any case.
Rename RawImage model to Binary and move it to Model namespace.
Introduce BinaryInterface. I've put it to Binary namespace as I think it is better place then Model one. What do you think?
small fixes and some new tests.

makasim · 2014-01-03T17:44:10Z

@havvg btw I think that current Loaders (FilesystemLoader or StreamLoader) could be moved out from Imagine namespace. To Binary\Loader for example.

makasim · 2014-01-06T10:05:32Z

@havvg ping

havvg · 2014-01-07T06:34:50Z

Binary/BinaryInterface.php

+    public function getFormat();
+
+    /**
+     * It must return same result as self::getContent method.


I don't see the benefits of this method being contracted by the interface. The interface already contracts this data on the getContent method. I would remove the __toString from the interface, it's a magic method, which should never be part of any interface.

havvg · 2014-01-07T07:20:51Z

Move SimpleMimeTypeGuesser and its itnerface to Binary namespace. It was in Imagine one and I think it is not the right place in any case.

Move Guesser by content to Binary namespace. It was in Imagine one and I think it is not the right place in any case.

Introduce BinaryInterface. I've put it to Binary namespace as I think it is better place then Model one. What do you think?

I'm not sure about the namespace. I would rather name it Domain as it provides basic implementations of the model. I did it wrong in my PR, too and will adapt it accordingly: interfaces go into the Model namespace and the implementations into Domain. The bundle itself relies on Model (the interfaces), but uses its Domain for easier usage (e.g. transforming a string from Data\LoaderInterface::find into Binary).

makasim · 2014-01-08T07:30:04Z

hm. I dont like Domain\Model concept but I will do

havvg · 2014-01-08T07:35:37Z

It's not a namespace Domain\Model, but two separate namespaces: Liip\ImagineBundle\Model and Liip\ImagineBundle\Domain. If you got another suggestion, I'm open to it :)

My suggestion is derived from e.g. Symfony\Component\Security\Acl.

makasim · 2014-01-08T07:40:34Z

yeah, I was about separate namespace, (just wrote it badly in the message). I would check Symfony\Component\Security\Acl and comment after.

makasim · 2014-01-08T10:05:29Z

@havvg found current approach with binary namespace is similar to where symfony put user and its interface. Its in not in the general namespace model but in the logic one. where acl security use other approach.

I am personally prefer how its done now but if you still want to domain\model way: let me know I update it

havvg · 2014-01-08T10:31:43Z

I see, then we are mixing multiple worlds currently (FOSUserBundle has similar approach but also mixes it up).
I don't see any clean line that's rather common, so let's keep it the way you currently got it and when we get more and more done, we can check the namespace structure again, if required.

makasim · 2014-01-08T12:53:58Z

and when we get more and more done, we can check the namespace structure again, if required.

👍

makasim · 2014-01-08T19:48:23Z

toString removed - formapro-forks@ee70a73

havvg · 2014-01-08T21:35:59Z

Nice, squash & rebase please, can't merge right now :-)

makasim · 2014-01-09T10:51:19Z

@havvg rebased&sqaushed

[1.0] Rework data loaders. Introduce mime type guesser.

makasim reviewed Dec 23, 2013
View reviewed changes

ASKozienko reviewed Dec 24, 2013
View reviewed changes

havvg reviewed Jan 3, 2014
View reviewed changes

havvg closed this Jan 3, 2014

havvg reopened this Jan 3, 2014

havvg reviewed Jan 7, 2014
View reviewed changes

[data loader] rework to return binary instance. add mime type guesser.

76864e4

havvg added a commit that referenced this pull request Jan 9, 2014

Merge pull request #291 from formapro-forks/mime-type-guesser

99c407f

[1.0] Rework data loaders. Introduce mime type guesser.

havvg merged commit 99c407f into liip:develop Jan 9, 2014

makasim deleted the mime-type-guesser branch January 9, 2014 12:08

makasim mentioned this pull request Mar 14, 2014

Ensure valid format #93

Closed

[1.0] Rework data loaders. Introduce mime type guesser. #291

[1.0] Rework data loaders. Introduce mime type guesser. #291

Conversation

makasim commented Dec 23, 2013

makasim commented Dec 23, 2013

Choose a reason for hiding this comment

makasim commented Dec 23, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented Dec 25, 2013

makasim commented Dec 27, 2013

havvg commented Dec 27, 2013

makasim commented Dec 27, 2013

makasim commented Jan 2, 2014

havvg commented Jan 3, 2014

Choose a reason for hiding this comment

makasim commented Jan 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havvg commented Jan 3, 2014

makasim commented Jan 3, 2014

makasim commented Jan 3, 2014

havvg commented Jan 3, 2014

makasim commented Jan 3, 2014

makasim commented Jan 3, 2014

makasim commented Jan 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havvg commented Jan 7, 2014

makasim commented Jan 8, 2014

havvg commented Jan 8, 2014

makasim commented Jan 8, 2014

makasim commented Jan 8, 2014

havvg commented Jan 8, 2014

makasim commented Jan 8, 2014

makasim commented Jan 8, 2014

havvg commented Jan 8, 2014

makasim commented Jan 9, 2014