Use an optional image dimensions cache

[ausi]

A single request in Contao causes many (hundreds) calls to Image::getDimensions(), e.g. for every icon shown on a backend page. A cache for the dimensions off all images would be a big performance boost.

@contao/developers does this implementation make sense that way?
If yes, I can make a pull request to the core-bundle to use the cache there.

[aschempp]

You should not add new methods to a stable interface, probably use a CacheableImageInterface?

Or maybe the methods dont need to be in the interface because they are not supposed to be used externally (except for dependency injection).

[aschempp]

Also, being only dependency injection would mean that the component can run with or without cache, which I think is a cool feature. But it needs a readme 😉

[ausi]

You should not add new methods to a stable interface

I think this should not be a problem if we release a new version 0.4.0.

probably use a CacheableImageInterface?

That would have the benefit to make the cache support for an implementor of the ImageInterface optional, but on the other hand it makes things more complicated and the library more bloated I think.

Or maybe the methods dont need to be in the interface because they are not supposed to be used externally (except for dependency injection).

The image library itself doesn’t create a cache, so the methods have to be used externally to make use of a cache.

Also, being only dependency injection would mean that the component can run with or without cache, which I think is a cool feature.

That’s the case already.

But it needs a readme 😉

I will add :)

[leofeyer]

You should not add new methods to a stable interface, probably use a CacheableImageInterface?

That's not a problem IMHO:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

[aschempp]

The image library itself doesn’t create a cache, so the methods have to be used externally to make use of a cache.

I don't understand that. If I do not setter-inject an image dimensions cache, the getDimensions method will simply fall back to not using a cache (because it's null). Therefore it's an optional dependency, and nothing that should be in the interface because it is not required for the functionality.

[ausi]

Take a look at Resizer.php:147 for example, it expects that ImageInterface::getDimensionsCache() is available, which can only be guaranteed if it’s in the interface. So either the ImageInterface gets the getDimensionsCache method or we add a new interface with that method which would bloat the library for no benefit.

If such a case happens in the future, going with a new interface would be the only option because of BC. But we don’t yet have to be BC with this library IMO.

[aschempp]

The resizer uses the concrete class, so it knows the method is there. The interface is for injection and methods to use, setting the cache layer is not relevant for that.

[ausi]

On that line: Resizer.php:147 $image->getDimensionsCache() the $image refers to the interface, not the concrete class.

[ausi]

An alternative from @aschempp: Remove getDimensionsCache and setDimensionsCache from the ImageInterface and add a ImageInterface::clone($path) that can be used by the resizer in Resizer.php:147, did I get that right?

I think this could be a better solution and more flexible for future additions.

[ausi]

@contao/developers what do you think about the clone() method?

[leofeyer]

I like it.

[ausi]

This is not the way we want to go, see contao/core-bundle#626 (comment)