Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

php: add bindings for PHP 7 #909

Closed
wants to merge 15 commits into from
Closed

php: add bindings for PHP 7 #909

wants to merge 15 commits into from

Conversation

adsr
Copy link
Contributor

@adsr adsr commented Feb 11, 2017

This has been on my todo list for a long time.

Most of weechat-php-api.c is code generated, so any bugs lurking within should at least be consistent / easy to fix. Manual testing runs clean through valgrind. The plugin could use more thorough testing overall though.

A note about script reloading: In PHP it is difficult to isolate the namespace for an individual script. (E.g., there is no equivalent of lua_State.) To avoid polluting global namespace, I suggest wrapping all user scripts in an anonymous class instance. (See test.php.) Named classes and top-level functions will work, but will error with "already declared" when reloading the script.

Another note: I have a pull request open to fix callbacks on anonymous class methods here: php/php-src#2381 Until that is merged users should do this:

weechat_hook_signal("*,irc_in2_privmsg", CLASS . '::' . 'onPrivMsg', '');

instead of this:

weechat_hook_signal("*,irc_in2_privmsg", [$this, 'onPrivMsg'], '');

EDIT: I wrote a work-around to address the above. See #909 (comment)

@flashcode flashcode added the feature New feature request label Feb 12, 2017
@flashcode
Copy link
Member

Hi,
Thanks for the PR, nice work!
I'll test that soon.

What's the minimum PHP version required?
I ask because I do many builds on old distribution versions (Debian/Ubuntu), so I need to know if old PHP versions can work too.

@adsr
Copy link
Contributor Author

adsr commented Feb 12, 2017

Min version is 7.0.0. Support for 5.x would require some refactoring and a hairier cmake script :)

@flashcode
Copy link
Member

@adsr: OK.
Could you please attach a testing PHP script, using some API functions (like hooks)?
Thanks.

@adsr
Copy link
Contributor Author

adsr commented Feb 12, 2017

@flashcode There is a test script here https://github.com/weechat/weechat/pull/909/files#diff-2ddb2c57b4fd43a1745f4692105df66c I added some more hooks to it and fixed a few things.

@flashcode
Copy link
Member

What would be the syntax if you don't use anon functions? (for example to declare many hooks with the same callback)

@adsr
Copy link
Contributor Author

adsr commented Feb 14, 2017

@flashcode Any callable type should work but I'd recommend this setup:

new class {
    function __construct() {
        ...
        weechat_hook_x(..., [ $this, 'callbackFunc' ], ...);
        weechat_hook_y(..., [ $this, 'callbackFunc' ], ...);
    }

    function callbackFunc(...) {
        ...
    }
};

@flashcode
Copy link
Member

I see.
The problem is that this syntax is different from all other scripting plugins, where only a string with the name of a function is enough.
Is it possible to do that with PHP?

@adsr
Copy link
Contributor Author

adsr commented Feb 14, 2017

There are a bunch of ways. Instead of [ $this, 'func' ] you can also use __CLASS__ . '::func' or 'self::func' which are intended for static methods but will actually work for instance methods with some caveats. Plain old 'func' works if you define a top level function func() {} but then you run into namespace collision on script reload. Here is another approach:

$cb = function (...$args) {
    weechat_printf(null, json_encode($args));
    return WEECHAT_RC_OK;
};
weechat_register('phptest', 'adsr', '0.1', 'GPL3', 'test', '', '');
weechat_hook_signal('*,irc_in2_privmsg', $cb, 'anything');
weechat_hook_completion('phpcompletion', 'desc', $cb, 'anything');
weechat_hook_command('phptestcmd', 'desc1', '', 'desc2', '', $cb, 'anything');
weechat_hook_config('weechat.look.item_time_format', $cb, 'anything');
weechat_hook_process('date', 10, $cb, 'anything');

Yet another way is to tweak the bindings so they automatically convert 'methodName' to a method pointer behind the scenes (instead of pointing to top level function 'methodName') if in class scope. That is non-standard though and would confuse PHP devs.

@adsr
Copy link
Contributor Author

adsr commented Mar 3, 2017

I added two functions forget_class and forget_function which (hackily) remove top-level classes and functions from their respective tables. This provides another alternative to use normal classes and functions, forget_* them in the plugin's shutdown function, and script reloading will work fine.

@adsr
Copy link
Contributor Author

adsr commented Mar 12, 2017

Rebased

@adsr
Copy link
Contributor Author

adsr commented Apr 17, 2017

I've been running a php bot on top of this plugin for several months. It doesn't exercise the entire api, but is stable with the parts it does use.

@adsr adsr force-pushed the plugin-php branch 2 times, most recently from fc3cbbb to bb44bc4 Compare April 26, 2017 23:07
@flashcode flashcode added this to the 2.0 milestone Jun 10, 2017
@adsr
Copy link
Contributor Author

adsr commented Jun 16, 2017

Rebased

@flashcode
Copy link
Member

Hi @adsr,
As this feature is planned for version 2.0, is it ready to merge now or are you still working on it?
I plan starting working on this merge (+ update docs, debian builds, ...) in the next days.

@flashcode
Copy link
Member

By the way, which PHP packages should I install on Debian (Sid) to compile the PHP plugin?

@adsr
Copy link
Contributor Author

adsr commented Aug 31, 2017

@flashcode Great. I am done working on it except for the CMake build script. The last few commits I made were an attempt to get the Travis build to compile the PHP extension via autotools, but it isn't working for some reason. Can we ask Travis for a debug VM to investigate? I will see if I can get a CMake build script working in the mean time.

Any version of PHP compiled with --enable-embed should work. On Debian I believe the package is libphp-embed for the library and php-dev for headers.

@flashcode
Copy link
Member

I tried with these PHP packages installed:

$ dpkg -l | grep php
ii  libphp-embed                              1:7.0+54                       all          HTML-embedded scripting language (Embedded SAPI library) (default)
ii  libphp7.0-embed                           7.0.22-3                       amd64        HTML-embedded scripting language (Embedded SAPI library)
ii  libphp7.1-embed                           7.1.8-1                        amd64        HTML-embedded scripting language (Embedded SAPI library)
ii  php-all-dev                               1:54                           all          package depending on all supported PHP development packages
ii  php-common                                1:54                           all          Common files for PHP packages
ii  php-dev                                   1:7.0+54                       all          Files for PHP module development (default)
ii  php7.0-cli                                7.0.22-3                       amd64        command-line interpreter for the PHP scripting language
ii  php7.0-common                             7.0.22-3                       amd64        documentation, examples and common module for PHP
ii  php7.0-dev                                7.0.22-3                       amd64        Files for PHP7.0 module development
ii  php7.0-json                               7.0.22-3                       amd64        JSON module for PHP
ii  php7.0-opcache                            7.0.22-3                       amd64        Zend OpCache module for PHP
ii  php7.0-readline                           7.0.22-3                       amd64        readline module for PHP
ii  php7.1-cli                                7.1.8-1                        amd64        command-line interpreter for the PHP scripting language
ii  php7.1-common                             7.1.8-1                        amd64        documentation, examples and common module for PHP
ii  php7.1-dev                                7.1.8-1                        amd64        Files for PHP7.1 module development
ii  php7.1-json                               7.1.8-1                        amd64        JSON module for PHP
ii  php7.1-opcache                            7.1.8-1                        amd64        Zend OpCache module for PHP
ii  php7.1-readline                           7.1.8-1                        amd64        readline module for PHP

pkg-config reports no php:

$ pkg-config --list-all | grep -i php

So WeeChat doesn't detect PHP and the plugin is not compiled.

Did I miss something?

@adsr
Copy link
Contributor Author

adsr commented Aug 31, 2017

Hm I'm not sure those packages install any pc files. The autotools script checks pkg-config first, then php-config (Debian packages should provide this), then AC_CHECK_HEADER/LIB[0]. It's possible the PHP_TEST program below this is failing for some reason?

[0] https://github.com/adsr/weechat/blob/2b8b56ec6d5b6d2a88b704c1cafe96642fb3524e/configure.ac#L805-L858

@flashcode
Copy link
Member

I'm testing exclusively with cmake, which is recommended over configure (autotools may be removed in a future version, they're here only for historical reasons).
So for now I'll try configure to check, but the file FindPHP.cmake must be fixed since it looks only for pkg-config.

@adsr
Copy link
Contributor Author

adsr commented Aug 31, 2017

I updated the CMake script to use php-config. I'm curious if that works on your setup. The Travis build seems to have multiple PHP versions installed because the build logs contain PHP 5 include directories. Currently the build scripts look for php-config, then php-config7.2, then php-config72, then the same for 7.1 and 7.0. I think tweaking that search order might help.

@adsr
Copy link
Contributor Author

adsr commented Aug 31, 2017

The autotools and CMake Travis builds compile the plugin successfully now.

@flashcode
Copy link
Member

Merged, thanks for this feature.

Now I'm testing all functions, I saw some potential problems, I'll check and fix if needed.

Not directly related to PHP, but I'm writing tests for the whole script API, that will generate scripts in each language, to test all functions.

@flashcode flashcode closed this Sep 3, 2017
@flashcode flashcode self-assigned this Sep 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants