Opened 2 years ago

Closed 18 months ago

Last modified 15 months ago

#817 closed enhancement (fixed)

Add PHP 8 support

Reported by: Ryan Schmidt Owned by: Olly Betts
Priority: normal Milestone: 1.4.22
Component: Xapian-bindings (PHP) Version: 1.4.19
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description

checking for /opt/local/bin/php-config81... /opt/local/bin/php-config81
checking /opt/local/bin/php-config81 version... 8.1.1 (PHP >= 8.0 isn't yet supported)
configure: error: /opt/local/bin/php-config81 reports version 8.1.1 - PHP7 bindings need 7.x

Change History (12)

comment:1 by Olly Betts, 2 years ago

That error was new in 1.4.19, but in 1.4.18 you'd have got an obscure compile error with PHP8 instead:

` error: cannot initialize a parameter of type 'zend_object *' (aka '_zend_object *') with an lvalue of type 'zval *' (aka '_zval_struct *') `

This is because PHP's C API has changed a bit in PHP8. We need SWIG to add support PHP8 - the current status is SWIG git master supports PHP8 but this is not in a stable SWIG release yet.

However there have been major changes in how SWIG-generated PHP wrappers work - instead of the generated .php script with class definitions, classes are created from C/C++ code using PHP's C API. These changes are definitely for the better as they fix a lot of problems with the old approach and the new generated bindings should also be faster. However it does mean updating our bindings is going to require more work than just using a new SWIG version since currently we post-process the generated wrapper script to add type declarations to function parameters and keep references to set objects.

When https://github.com/swig/swig/issues/2027 is merged that will automatically add type declarations for PHP8. I'm not sure what to do for PHP7 though - the type declaration API is very different there (it had to be reworked to allow PHP8 to support things like int|object). Perhaps we just leave the PHP7 bindings as-is (and drop them in the next release series - PHP7.4's EOL is only ~10 months away now). The awkward part is that probably means we would need two different SWIG versions to generate bindings for PHP7 and PHP8. Maybe we can post-process the generated C++ wrapper code to add them. Or perhaps just drop PHP7 support in the 1.4.x release that adds PHP8 support and tell people who still want PHP7 support to use the last 1.4.x xapian-bindings which had support.

I'm hoping we can deal with keeping references to set objects using custom typemaps or maybe %extend but I've not tried yet.

Also https://github.com/swig/swig/issues/2125 really needs fixing.

So PHP8 support is coming, but it may be a while away yet I'm afraid.

comment:2 by Olly Betts, 2 years ago

I've managed to merge the changes to address those two issues.

I'll try to get PHP8 working with Xapian git master as a next step as that may shake out some bugs in the new SWIG PHP8 support, and it'd be helpful to do that in time to get them fixed before SWIG 4.1.0 is released.

comment:3 by Olly Betts, 2 years ago

Found and fixed one SWIG bug so far: https://github.com/swig/swig/commit/d43f28a6664561b0952722d51702d5e99a806918

Currently at a segfault which I assume is due to the script to add references to passed in functor object no longer doing anything.

comment:4 by Andy Postnikov, 2 years ago

Is there a way to build bindings for php7 and php8 same time?

As maintainer of PHP packages in Alpinelinux I need to build both bindings to allow dependent packages to make transition.

As I get I need to wait for 1.4.20 release to do testing, is there a set of patches to apply to test it for all supported arches before new release?

Related https://gitlab.alpinelinux.org/alpine/aports/-/issues/13461

comment:5 by Olly Betts, 2 years ago

Is there a way to build bindings for php7 and php8 same time?

There isn't a way to build working php8 bindings at all yet. Once there is it's unlikely that there will be a 1.4.x release which support both PHP7 and PHP8. That may not be ideal, but please understand it's not easy to support both in one xapian-bindings release.

SWIG 4.1.0 brings a much needed major change in how SWIG generates PHP class wrappers - until now SWIG generated wrappers for a load of "flat" functions in C code and also generated a .php file containing class definitions with methods which forward to those flat functions. There have been a lot of corner case bugs with this over the years, many of which never got fixed because they were too hard. Now SWIG will generate the classes and their methods directly using PHP's C API. This has fixed a whole slew of bugs and should be faster too.

However, currently in Xapian's PHP bindings we post-process the xapian.php wrapper to add PHP type declarations (aka "type hints") and also to add code to keep references to set objects (so e.g. XapianQueryParser::set_stemmer() keeps a reference to the currently set stemmer). We can't post-process xapian.php to do this now because the classes and methods aren't defined there any more.

I don't have a solution for the ownership tracking yet, but maybe #714 offers an answer. Once we solve this I'd expect that solution would work for PHP7 too.

But as for type declarations, SWIG 4.1.0 will automatically generate those for us but only for PHP 8. PHP7 type declarations are much more limited (you can only have one type, optionally nullable, whereas PHP8 allows e.g int|object) and as a result the C API for this changed in PHP8. For SWIG we concluded it didn't make sense to implement support for the limited old-style type declarations when PHP7 is 10 months from end of life). So SWIG generates a single wrapper you can use with PHP7 or PHP8 but it doesn't have type declarations for PHP7. That's reasonable in general, but for Xapian it's means the PHP7 bindings would lose the type declarations we currently add, which would be a regression.

So the easiest approach from the Xapian viewpoint is to have a hard switch from PHP7 to PHP8. Anything else either requires that we have to use two different SWIG versions to generate a single release and two sets of the interface files, etc, or to add support for PHP7 type hints to SWIG.

I can see that might be awkward for a Linux distro shipping packages, but at least binary packages aren't really feasible currently due the GPL vs PHP licence incompatibility (see #191). Do you not distribute binary packages for the PHP bindings? Or are you ignoring this licensing problem with doing so? The next Xapian release series should address that by relicensing to MPL, but 1.4.x contains a lot of code we can't relicense.

I'm already fighting a significant backlog of things I haven't got to yet, so I simply can't commit to doing extra work simply to support PHP7 and PHP8 in one release. I've already put in a lot of work just to support PHP8 at all, and there's still more to go there.

If you need to manage a transition, my suggestion would be to create a "xapian-bindings-php7" source package which is just the last xapian-bindings 1.4.x version before the switch to PHP8.

comment:6 by Andy Postnikov, 2 years ago

Thank you a lot for detailed answer!

Now it's clear that hard switch is preferred, moreover according to https://www.php.net/supported-versions.php 7.4 will be phased out this year, so no reason to put efforts into backward compatibility.

Thanks for idea with BC source package - that's easy doable and will not delay Alpine release!

comment:7 by Olly Betts, 23 months ago

Owner: set to Olly Betts
Status: newassigned

Indeed - PHP 7.4 is already out of active support, and upstream security support ends in ~5.5 months.

I've now managed to get PHP 8 support working with Xapian git master. I've not yet merged it, but hope to this coming week, which should at least allow people to start testing this.

There isn't yet a stable SWIG release with this support though, and in the process of doing this I found a few bugs in xapian-bindings which affect 1.4.19, so I'm currently thinking to make a 1.4.20 release very soon without the PHP8 support but fixing these bugs, then make a 1.4.21 with the PHP8 support (and dropping PHP7) a month or two later once we're had more of a chance to find bugs in the new support, and hopefully there will be a stable SWIG release by then (not a hard requirement, but I'm a bit concerned about generating a release so far into a Xapian stable release branch with a git snapshot of SWIG as there's a risk of it breaking other languages we use SWIG for, and reducing that risk is good).

comment:8 by Olly Betts, 19 months ago

Status update:

SWIG 4.1.0 will add PHP8 support and https://sourceforge.net/p/swig/mailman/message/37710392/ said two weeks ago:

All going well, I propose to push out a release candidate in the first week of October and the full 4.1.0 release a week or two afterwards.

It looks like things are going OK - there are still a few tickets and patches on the milestone, but the list is diminishing.

I've just pushed 80c8f86dfc2576a14171d33679d5d21ba886b8dd to Xapian git master which switches the PHP bindings to be for PHP8 (and currently pulls in a suitable git snapshot of SWIG to generate them).

Next I'll work on backporting that to the RELEASE/1.4 branch.

Once SWIG 4.1.0 is out, I'll update to git master use that and backport that change too.

That means this is almost certain to be in xapian-bindings 1.4.22 (the next 1.4.x release). I pencilled that in for "December 2022" when I updated RoadMap after releasing 1.4.21, but I'll probably bring that forward assuming SWIG releases this month and nothing turns up which is a release blocker.

comment:9 by Olly Betts, 19 months ago

Milestone: 1.4.22

Backported for 1.4 in 0577783bed3597bfa13cafe5396f4fcad5b36392.

Leaving open for now as we still need to update to use SWIG 4.1.0 once released.

comment:10 by Olly Betts, 18 months ago

Resolution: fixed
Status: assignedclosed

SWIG 4.1.0 has been released and I've updated to use that and backported.

comment:12 by Olly Betts, 15 months ago

Sorry it's dragged on, but it should be soon - I'm just trying to get everything lined up suitably for a release (some of which is important for PHP8 support - e.g. I fixed a problem with PHP 8.2 only a few days ago). Likely early next month if not this month.

comment:13 by Olly Betts, 15 months ago

The snapshot tarballs are now working again, so using xapian-bindings from here should give working PHP8 bindings without having to bootstrap a working build tree from a git checkout:

https://oligarchy.co.uk/xapian/RELEASE/1.4/

I'd encourage people who are interested in PHP8 support to test this more widely to help shake out any problems. It passes the automated tests we have, but they're certainly not exhaustive.

Note: See TracTickets for help on using tickets.