search

libvips / php-vips

php binding for libvips
MIT License
615 stars 25 forks source link

Move FFI functions to a separate class #147

Closed kleisauke closed 2 years ago

kleisauke commented 2 years ago

This makes it easier to check whether php-vips is based on FFI or php-vips-ext at runtime.

<?php

require __DIR__ . '/vendor/autoload.php';

use Jcupitt\Vips\FFI;

echo \class_exists(FFI::class) ? "php-vips based on FFI" : "php-vips based on php-vips-ext";

/cc @chregu since these checks needs to be adjusted after this PR: https://github.com/rokka-io/imagine-vips/blob/a70db9c3fc678fcb3b70c99682e15ad45325f27f/lib/Imagine/Vips/Imagine.php#L53 https://github.com/rokka-io/imagine-vips/blob/a70db9c3fc678fcb3b70c99682e15ad45325f27f/lib/Imagine/Vips/Imagine.php#L208

kleisauke commented 2 years ago

Oh wait, the above mentioned checks need to be modified anyway, because the ffi() function has been renamed to vips() in https://github.com/libvips/php-vips/commit/d8d0d6336a9b7c2de4d7b379eef440c9c918cc90. Sorry about that.

kleisauke commented 2 years ago

... perhaps we need to find another name for this new class, since the FFI class is already used by the FFI extension?

chregu commented 2 years ago

The FFI class in this case is namespaced, so that shouldn't be an issue?

chregu commented 2 years ago

The problem in imagine is more that there is already 2.0.3 released, so I need to check for all cases. (Or just exclude 2.0.0 - 2.0.3 in composer.json, which shouldn't be an issue to do, because I don't think anyone needs <= 2.0.3 but not > 2.0.3) But I guess it would be easier for this check, if the next version would be just 2.1.0 and not 2.0.4

jcupitt commented 2 years ago

Ooop, I'd forgotten this PR, sorry. Should we fix this up and release as 2.1?

It might be useful to get something like this patch in too (look for libvips in a few standard locations): https://github.com/libvips/php-vips/issues/140#issuecomment-1271481353

kleisauke commented 2 years ago

No worries, it fell off my radar too. I'll rebase and update for inclusion in version 2.1.

It might be useful to get something like this patch in too (look for libvips in a few standard locations): #140 (comment)

Ah, I think it's due to Apple's security restrictions, where DYLD_* environment variables are purged when starting protected processes - see: https://github.com/Homebrew/brew/issues/13481#issuecomment-1168598570.

Perhaps we could reinstate the removed VIPSHOME logic in commit https://github.com/libvips/php-vips/pull/133/commits/387f4c18e0ea80bc1e5fd3c7ad80499a8a8e119f instead? Allowing macOS M1 users to run php-vips programs using:

export VIPSHOME=$(brew --prefix vips)

The FFI class in this case is namespaced, so that shouldn't be an issue?

Indeed, that shouldn't be an issue (I hadn't been working on any PHP-related code lately).

kleisauke commented 2 years ago

... included that patch with some modifications in commit 0b29a6e97869eb5a1f6b2fff965d4b570437d7dd, but let me know if you prefer that in a separate PR.

jcupitt commented 2 years ago

That all looks great! I'll make 2.1.

chregu commented 2 years ago

Just for the record. I updated https://github.com/rokka-io/imagine-vips with those additions (and excluded php-vips 2.0 in composer.json to make life easier in the code)

kleisauke commented 2 years ago

@jcupitt v2.1.0 was tagged on commit https://github.com/libvips/php-vips/commit/c46db5539768b132a58f18c88790541b5ebbe440, but that commit is not present in the master branch. Probably this is due to git push --tags.

jcupitt commented 2 years ago

Ack I'm always doing that, the behaviour is so confusing, sorry.