rokka-io / imagine-vips

libvips adapter for php imagine
https://rokka.io
Other
41 stars 8 forks source link

#issue-13, wrong return types in Image::save and Layers::coalesce, en… #14

Closed batyrmastyr closed 1 year ago

batyrmastyr commented 4 years ago

…forced return types in other places. #13

erickloss commented 1 year ago

Hi :) is this going to be merged? Cheers

chregu commented 1 year ago

Sorry about that, it got lost somehow:

Answer: The extended from interfaces from imagine/imagine don't have return types neither, introducing them now would be quite the BC break for people extending from it. But open to discuss it, nevertheless

dbu commented 1 year ago

it is "only" a BC break when extending the classes, i don't know how much people do that. (one of the reasons to make classes final, if they were final we would be able to do this without worry).

however, as code should probably reason on the imagine interfaces rather than the implementations here, the return types would be most valuable on the base imagine interfaces. which definitely is a BC break and needs a new major version, and if they do that, we can do a new major to adjust the return types.

if we add return types without the interfaces having them, we also risk that we would have to change some type if imagine decides to do something slightly differently.

chregu commented 1 year ago

I added now phpstan tests, if imagine should change something (which I hardly expect without a major change), we will realize it in those tests.

I do extend the Imagine/Image cases to handle some special cases within rokka, so there are usecases for not making them final.

I had to do some BC break in #27, to make it run on PHP 8.1 without introducing ugly hacks. Since we're still in pre 1.0 phase, we're allowed to break BC (but I of course prefer to not to do that too often or too breaking)

chregu commented 1 year ago

I close this for now. If something still should be changed, please open a new report. Thanks for your contribution