search

libvips / php-vips-ext

Low-level libvips binding for PHP
MIT License
101 stars 11 forks source link

Release 1.0.11 test fail on 32-bits #38

Closed andypost closed 4 years ago

andypost commented 4 years ago

Just used to upgrade to 1.0.11 and test tests/042.phpt started to fail 32-bits on x86 and armv7

vips-1.0.11/tests$ cat 042.diff 
001- pass set_metadata
002+ 
003- pass get_metadata
003+ Notice: Trying to access array offset on value of type int in /mnt/community/php7-pecl-vips/src/vips-1.0.11/tests/042.php on line 21

It means that it fails somehow according to https://github.com/libvips/php-vips-ext/blob/master/vips.c#L1636

PS: here's CI runs

jcupitt commented 4 years ago

Hello @andypost,

Thank you for the report. It looks like long on 32-bit PHP installations is only 32-bit, but GType (the thing returned by vips_type_from_name("VipsBlob")) is 64.

I think we have to change the type of vips_image_set_type() from:

long vips_image_set_type(resource image, integer gtype, string field, mixed value)

To:

long vips_image_set_type(resource image, string type, string field, mixed value)

ie. we put the string -> GType conversion into set_type. It'll be a bit slower, but it avoids exposing GType to PHP.

We could allow both integer and string, of course, perhaps that's the best solution. What do you think?

jcupitt commented 4 years ago

I went for string names for types. Hopefully this fixes it.

Thanks again for the report!

andypost commented 4 years ago

Thanks, gonna try patch today

andypost commented 4 years ago

1.0.12 now pass all https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/11839/pipelines

jcupitt commented 4 years ago

That's great! Thank you for testing.