FEATURE: Only output existing images and allow upscaling

[manuelmeister]

This is a draft pr to limit srcset output to only existing images.

I'm not sure I feel comfortable merging it like this. Maybe we first need a PR to add tests, so we can ensure that everything still works correctly.

Fixes #33

[mficzel]

I like the idea, more precisely i think the biggest generated image should be the largest possible image with the required aspect ratio if there is an aspect ratio defined, otherwise the original one.

[manuelmeister]

That's exactly what I've done.

I've also increased the minimum php version to 7.4 as it did not support an extended return type.

[mficzel]

@manuelmeister thanks a lot for the pr. Need to find some time and mindset for a proper review as this is non trivial.

[manuelmeister]

@mficzel Can you have a look again?

[mficzel]

I do not like the allowUpscaling argument that is passed around all over the place and adds quite a bit of complexity while i think that upscaling general is not a good idea. Also this pr does some unrelated things that look valuable but should not be mixed in this one.

How about this approach:

1. Add a method. supportsUpscaling(): bool to the scalableImageSourceInterface
2. Asset Image source should return false while DummyImageSource should return true
3. Skip srcset items that would yield a bigger image if the CurrentImageSource does not supportsUpscaling

That way we have strong defaults and anyone that has a need for upscaling images can still create a custom AssetImageSource

[mficzel]

@manuelmeister i talked with @bweinzierl at the contribution day and he is willing to help here aswell.

BTW: A thing that is important to me is that the pr only does what is mentioned. The cleanups you added are nice but make reviewing harder. It would be great to split those topics into separate prs.

[manuelmeister]

@mficzel I agree with your comments.

[manuelmeister]

@mficzel I removed the aspect ratio part from this PR and opened a new one: #77

[mficzel]

I like this now. However do we really have to add the "supportUpscaling" flag? I see no real user case for that and would leave this out and always assume allowUpscalinj = false. Just to make the tests for us a little bit easier.

[manuelmeister]

For a project we needed the image upscaling, to ensure the images are the same.

[mficzel]

I generally approve. This makes sense and i am fine with the structure. Will do some extra testing before merging to be extra sure this is as non breaky as possible,

Thanks for the pr.