final classes

[ShurikAg]

Hi,

We are using this library for our project and we came across an issue that blocks us from extending the functionality and in particular the fact that quite a few classes defined as final.

What is the actual reason for these classes to be final?

[avalanche123]

Sure, here is a blog post where I summarize it http://avalanche123.com/blog/2011/03/15/how-to-write-clean-code/

[ShurikAg]

There are some discrepancies in what you are saying in your post. Well, how can you talk about extendible if all of your public interface is final???

Quote (http://php.net/manual/en/language.oop5.final.php) : "If the class itself is being defined final then it cannot be extended."

And by the way it have nothing to do with TDD. You testing your implementation, I, as a client of your library will test it's extensions. Extending a class doesn't mean changing the existing one...

[avalanche123]

Can you elaborate on why you need to extend any of classes I provide?
Also, inheritance is not the only way to extend functionality, all final classes in Imagine have a corresponding interface. I encourage you to implement these interfaces in your own classes if you need to change behavior of default implementations.
http://en.wikipedia.org/wiki/Composition_over_inheritance

[romainneutron]

Hello @ShurikAg

There are many ways to extend the library, inheritance is one of them, composition is another way.
Why don't you explain you actual problem / project ?

[ShurikAg]

OK, there is a case in out app that we need to have some more details in the Image object (such as original mimeType) for further validation purposes.
Of coarse, we could do some workarounds, and pass get the mimeType from somewhere else, but in our case we designed a validator engine based on decorator design pattern and this engine is build to accept Image object.

@avalanche123 we don't need to change the functionality, we need to add/extend it... That is what extension is about, right?

[avalanche123]

@ShurikAg please refrain from snarky comments if you want to keep this conversation productive and have this issue resolved.

To resolve your issue, please provide some more details on how you are accomplishing this task - show me how you would like to extend the image class so we can work out if this is a case where we need to reopen the class for extension or if we can use composition instead.

If you feel decisions I made while building Imagine were wrong, there were, what I believed to be, good reasons for them. It might be time to re-evaluate them.

P.S. my previous comment above said "extend functionality", not "change functionality" as you've read it.

[ShurikAg]

Sure, what I'm trying to do is to add a single private field into the Image type of object and getter and setter methods.

The way I initially planned o do it is by extending an Image class.
Now, in order to make this work I would also extend Imagine class and overwrite wrap method to return CustomImage, but wrap is private...

Ideally, it would be nice to have an ability to inject configuration of the Image class name to create (or something similar).

At this point I am forced to create a wrapper class containing and Image object in it as a property. But, in this case, instead of simply operating on the public interface of the Image class I have to either get the image object from the wrapper or create a proxy methods for each and every method I might need for the Image class.

If I could easily change the object type that gets created by the Imagine class (of coarse, been ImageInterface) I could use the same object as is within the context of our app without adding any complexity...

Hoep that helps...

[avalanche123]

Here is how I would do it:

<?php

interface MyImageInterface
{
    function getMimeType();
}

class MyImageClass implements MyImageInterface, Imagine\Image\ImageInterface
{

    private $mimeType;
    private $image;

    public function __construct(Imagine\Image\ImageInterface $image, $mimeType)
    {
        $this->image = $image;
        $this->mimeType = $mimeType;
    }

    public function getMimeType()
    {
        return $this->mimeType;
    }

    public function get($format, array $options = array())
    {
        return $this->image->get($format, $options);
    }

    public function __toString()
    {
        return $this->image->__toString();
    }

    public function draw()
    {
        return $this->image->draw();
    }

    public function effects()
    {
        return $this->image->effects();
    }

    public function getSize()
    {
        return $this->image->getSize();
    }

    public function mask()
    {
        return new MyImageClass($this->image->mask(), $this->mimeType);
    }

    public function histogram()
    {
        return $this->image->histogram();
    }

    public function getColorAt(PointInterface $point)
    {
        return $this->image->getColorAt($point);
    }

    public function copy()
    {
        return new MyImageClass($this->image->copy(), $this->mimeType);
    }

    public function crop(PointInterface $start, BoxInterface $size)
    {
        $this->image->crop($start, $size);

        return $this;
    }

    public function resize(BoxInterface $size)
    {
        $this->image->resize($size);

        return $this;
    }

    public function rotate($angle, Color $background = null)
    {
        $this->image->rotate($angle, $background);

        return $this;
    }

    public function paste(ImageInterface $image, PointInterface $start)
    {
        $this->image->paste($image, $start);

        return $this;
    }

    public function save($path, array $options = array())
    {
        $this->image->save($path, $options);

        return $this;
    }

    public function show($format, array $options = array())
    {
        $this->image->show($format, $options);

        return $this;
    }

    public function flipHorizontally()
    {
        $this->image->flipHorizontally();

        return $this;
    }

    public function flipVertically()
    {
        $this->image->flipVertically();

        return $this;
    }

    public function strip()
    {
        $this->image->strip();

        return $this;
    }

    public function thumbnail(BoxInterface $size, $mode = self::THUMBNAIL_INSET)
    {
        return new MyImageClass($this->image->thumbnail($size, $mode), $this->mimeType);
    }

    public function applyMask(ImageInterface $mask)
    {
        $this->image->applyMask($mask);

        return $this;
    }

    public function fill(FillInterface $fill)
    {
        $this->image->fill($fill);

        return $this;
    }
}

class MyImagine implements Imagine\Image\ImagineInterface
{
    private $imagine;

    public function __construct(Imagine\Image\ImagineInterface $imagine)
    {
        $this->imagine = $imagine;
    }

    public function create(BoxInterface $size, Color $color = null)
    {
        // determine image's mime type here
        $mimeType = "some mime type";

        return new MyImageClass($this->imagine->create($size, $color), $mimeType);
    }

    public function open($path)
    {
        // determine image's mime type here
        $mimeType = "some mime type";

        return new MyImageClass($this->imagine->open($path), $mimeType);
    }

    public function load($string)
    {
        // determine image's mime type here
        $mimeType = "some mime type";

        return new MyImageClass($this->imagine->load($string), $mimeType);
    }

    public function read($resource)
    {
        // determine image's mime type here
        $mimeType = "some mime type";

        return new MyImageClass($this->imagine->read($resource), $mimeType);
    }

    public function font($file, $size, Color $color)
    {
        return $this->imagine->font($file, $size, $color);
    }
}

Does this make sense?

[ShurikAg]

It does and that is exactly what I eventually did, but I am just not sure if that is the right way to do it if it could be much simpler by extending the class?

[avalanche123]

I agree, that PHP doesn't make it easy to build typed proxies, I keep image implementations final to be able to change them without breaking third party code that relies on existing implementation. I am open to changing this if enough people weigh in

[romainneutron]

Hello, another way to proxify would be such code :

public function __call($method, $args)
{
    return call_user_func_array(array($this->image, $method), $args);
}

Romain

On 15 nov. 2012, at 09:06, Bulat Shakirzyanov notifications@github.com
wrote:

Here is how I would do it:

image = $image; $this->mimeType = $mimeType; } public function getMimeType() { return $this->mimeType; } public function get($format, array $options = array()) { return $this->image->get($format, $options); } public function __toString() { return $this->image->__toString(); } public function draw() { return $this->image->draw(); } public function effects() { return $this->image->effects(); } public function getSize() { return $this->image->getSize(); } public function mask() { return new MyImageClass($this->image->mask(), $this->mimeType); } public function histogram() { return $this->image->histogram(); } public function getColorAt(PointInterface $point) { return $this->image->getColorAt($point); } public function copy() { return new MyImageClass($this->image->copy(), $this->mimeType); } public function crop(PointInterface $start, BoxInterface $size) { $this->image->crop($start, $size); return $this; } public function resize(BoxInterface $size) { $this->image->resize($size); return $this; } public function rotate($angle, Color $background = null) { $this->image->rotate($angle, $background); return $this; } public function paste(ImageInterface $image, PointInterface $start) { $this->image->paste($image, $start); return $this; } public function save($path, array $options = array()) { $this->image->save($path, $options); return $this; } public function show($format, array $options = array()) { $this->image->show($format, $options); return $this; } public function flipHorizontally() { $this->image->flipHorizontally(); return $this; } public function flipVertically() { $this->image->flipVertically(); return $this; } public function strip() { $this->image->strip(); return $this; } public function thumbnail(BoxInterface $size, $mode = self::THUMBNAIL_INSET) { return new MyImageClass($this->image->thumbnail($size, $mode), ``` $this->mimeType); } ``` public function applyMask(ImageInterface $mask) { $this->image->applyMask($mask); return $this; } public function fill(FillInterface $fill) { $this->image->fill($fill); return $this; }} ``` class MyImagine implements Imagine\Image\ImagineInterface{ private $imagine; ``` public function __construct(Imagine\Image\ImagineInterface $imagine) { $this->imagine = $imagine; } public function create(BoxInterface $size, Color $color = null) { // determine image's mime type here $mimeType = "some mime type"; return new MyImageClass($this->imagine->create($size, $color), ``` $mimeType); } ``` public function open($path) { // determine image's mime type here $mimeType = "some mime type"; return new MyImageClass($this->imagine->open($path), $mimeType); } public function load($string) { // determine image's mime type here $mimeType = "some mime type"; return new MyImageClass($this->imagine->load($string), $mimeType); } public function read($resource) { // determine image's mime type here $mimeType = "some mime type"; return new MyImageClass($this->imagine->read($resource), $mimeType); } public function font($file, $size, Color $color) { return $this->imagine->font($file, $size, $color); }} ``` Does this make sense? — Reply to this email directly or view it on GitHubhttps://github.com//issues/162#issuecomment-10400262.

[ShurikAg]

@romainneutron Sure, but this is kind of PHP "black magic" that I am personally trying to avoid...

[avalanche123]

magic methods won't work as you won't be able to implement correct interfaces unless you have those methods defined explicitly

[ShurikAg]

This is also correct...

[ShurikAg]

Here is another argument:
If you change the interface I will be forced to update my class, which could be avoided if the class was simply extendable.

[orfin]

We also have some issues according to "all-final" design of your classes.

In one of our project we want to resize image but without interpolation. So we looked at your Imagine\Gd\Image::resize method:

/**
     * {@inheritdoc}
     */
    final public function resize(BoxInterface $size)
    {
        $width  = $size->getWidth();
        $height = $size->getHeight();

        $dest = $this->createImage($size, 'resize');

        imagealphablending($this->resource, true);
        imagealphablending($dest, true);

        if (false === imagecopyresampled($dest, $this->resource, 0, 0, 0, 0,
            $width, $height, imagesx($this->resource), imagesy($this->resource)
        )) {
            throw new RuntimeException('Image resize operation failed');
        }

        imagealphablending($this->resource, false);
        imagealphablending($dest, false);

        imagedestroy($this->resource);

        $this->resource = $dest;

        return $this;
    }

In fact we could acomplish that by changing that single line:
if (false === imagecopyresampled($dest, $this->resource, 0, 0, 0, 0,
to
if (false === imagecopyresized($dest, $this->resource, 0, 0, 0, 0,

but due to "final" of that class and methods we are NOT able to extend it.

Also using composition pattern is not possible in that case - we won't be able to get $this->createImage(...), and $this->resource.

Is the only way to accomplish that is to copy code of the whole Image class to our own AND create custom ImagineInterface which will return our Image object?!
If so, we will simply have to move to an other library - managing such 'clones' of code is unacceptable (also keeping in mind that such design will block us in the future).

[romainneutron]

Hello,

What's the use case of using imagecopyresize instead of imagecopyresampled ?

Is it related to performance ?

[orfin]

No, it's related to the type of scaled image. It must not be interpolated to preserve original 'style'.
It's an QR Code - after interpolation it looks fuzzy. Also think about any bar codes etc. Resizing must preserve "sharp-pixels".

[orfin]

Ping @avalanche123

[romainneutron]

What about adding a second argument to the resize method ?

final public function resize(BoxInterface $size, $method = RESIZE_LANCZOS) for example ?

[romainneutron]

This method could be called like this :

$image->resize($size, ImageInterface::RESIZE_NONE);

that could be implemented through imagecopyresize in Gd driver

[tomkyle]

Also would like to open some of the final classes. In my app, I would like to pass a custom width and height parameter to the Box ctor, or at least allow only one parameter to be passed. So, lend me your ears –

Use case: My URL design allows for image size requests like /300x200px/path/to/img routed by Slim. Slim afaik does not allow slicing URLs by anything else other than /, so it will give me 300x200px part as my desired thumbnail size. I would like to use this in an derived MyBox class and parse-split it inside it. So: whilst the BoxInterface method implementation in Box is completely OK, it is the ctor I want to change.

Having read avalanche123's “clean code” blogpost and this discussion so far, the current class design forces me to copy all the original method implementations. IMHO there are two possibilities:

a) Remove the final keyword from current Box implementation.
b) Leave that final in the original class, but extract basics such as getWidth and getHeight, (but not the ctor) to a new abstract one, BoxAbstract, since this is what “abstract boxes usually do”.

Proposal b) would respect valanche123's view on wether-to-finalize code as well as giving others the flexibility of extending things.

(Disclaimer: I am not very firm with Git's issue management, so please forgive if I am too late here or in the wrong issue thread.)

[antokara]

I am not sure if there is another way to add the same functionality but my problem is that I cannot add extra effects besides the basic gamma, negative, grayscale, colorize and sharpen.

Imagick provides a ton of extra effects such as blur, vignette, etc. which are needed in my project and I could not find a way to add those effects without changing the imagine code due to the final class design.

My only solution was to use the getImagick() function and apply the effect there but I believe it would be ideal if I could extend the effects function to my liking and use that with the library somehow.

Perhaps I am wrong, just thought I should share my scenario and solution.
Thanks.

[stof]

I don't understand why you want to extend the class to apply the effects. Effects are provided as filters applied on an Image instance, not as methods on the Image itself.

[antokara]

Let me give you an example:

from the documentation

$image = $imagine->open('portrait.jpeg');
$image->effects()->gamma(0.7);
$image->save('negative-portrait.png');

Now the gamma effect is a function in the effects class which implements the interface.
Lets say I want to create an effect that combines a number of filters but don't want to repeat the code outside the effects class.

$image->effects()->myCustomEffect();

I hope the example is straightforward

[stof]

@antokara you should implement a filter actually.

[mlocati]

Me too I'd like to remove the final classes/methods.
We can follow semver if we have to break the backward compatibility.
So, I'll implement this before releasing the next version of Imagine (which should be 1.x) if there won't be any downvote from the other maintainers.

[mlocati]

Some classes are no more final, and others may be overridden by using the instance factory