-
Notifications
You must be signed in to change notification settings - Fork 17
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
Please update for PHP80 #5
Comments
Even if it builds, the arginfo definitions are missing:
|
I note there are two pull requests open for this, but neither has been merged yet, how can we make progress here and is there anything we can do to help? Never worked with a php/pecl module yet, so if there is a guide on how to approach this, or pointers, I'd be happy to see if we can dedicate resources on this. Lots of info out there on how to migrate PHP code itself ... but I can't find anything on the modules themselves. Happy to assist with both code and testing. |
HI, try this. Get the source of this package ready for php 8 Unpack and build with php tools for php 8 or php.81. Load the extension on your own php.ini file. |
Which is the archive taken from https://github.com/UNETVALE/php-radius |
OK, neither #6 or #9 is working for me. Trying to apply directly on top of the archive at https://pecl.php.net/get/radius-1.4.0b1.tgz #6 states as the first commit that it's importing that archive, so skipping over the first I get similar compile errors to #9. #9 doesn't actually fix PHP8 compatibility and after applying it the build still only works for PHP7.4, not for PHP8.0 or PHP8.1. Unfortunately I'm not permitted to just grab and use the archives as proposed. |
I have just merged #9, it would be great if someone can do some testing and file some PRs if there are issues. I have initially created this package, but have not worked on this more almost 20yrs, though I have not the permissions to transfer the ownership, I can merge PRs. |
The news is not good. At least not for php8.1. make.log config.log is generated by ./configure, configure.log is the CLI output from ./configure. Similar failures for php8.0. |
The same errors as @jkroonza reported in Ubuntu 22.04.1 LTS.
|
I have a working patch for PHP 8.0.12, I will see about getting corporate permission to provide it here. |
Here's my php 8 patch |
And the one Gentoo ended up using, which is what was merged (as I understand) + a few other changes to work around the errors I encountered. This applies on top of the archive from pecl for 1.4.0. I can confirm it works for 8.0 and 8.1, and according to the person that wrote it for 8.2 as well, but I'm not in a position to test that. DISCLAIMER: I did not write this, the commit entry that added this into Gentoo reads as follows:
|
I have applied the patch above to a new php8 branch. Could you please check if the php8 branch has a working codebase? |
I was unable to build foi master too. Using my own repo with my fixes, was solid good. |
My patch applies to 1.4.0b1 available from pecl.php.net, as this is what FreeBSD includes in their ports tree. Sorry, I'm not sure how that relates to the master branch here. |
Oh, also, FreeBSD distributes a patch that removes all the TSRMLS stuff, so I did not address that in my patch. |
Sorry, there are many patches around, it gets a bit confusing. There is a branch 1.4.0b1, those patchset is meant to be applied to this branch? Anyway, a clean well tested PR would be great. |
And the removal of the TSRMLS stuff was not present in the merged #9 so it should be applied to master as well. |
My understanding is that it's best to create a |
That is another way of dealing with the stubs. But it does not solve the issue at the moment: making master compatible with PHP 8.2. For this the TSRMLS stuff has to be removed first. So d2d10e1 has to be applied to master now. I will create a new PR for that. Afterwards you could always create a pull request to generate the arginfo.h files with gen_stub.php. Posting patches in a *.gz form in this issue is not helpful at all. Use a pull request if you want to update this repo. |
New PR: #11 |
merged #11, pls test |
php8.2:
|
Could you post the definition of php_combined_lcg in |
hmm, there must be a build issue with different php versions on my system. What I did was:
But configure sets:
|
update-alternatives --config php-version did the trick now, at least for php7.4, I'm getting this error:
same error with php8.0 and php8.1 |
I will check in Windows what happens if we change those calls to php_combined_lcg into php_combined_lcg(void). Maybe in older PHP versions the extra arguments are really needed. |
Even in PHP 7.0 it was php_combined_lcg(void). Looks like changing those calls is safe, but I will test it and provide another PR if compilation on Windows still succeeds. |
Removing the NULL in the calls to php_combined_lcg() seems the way to go: See also this comment: #5 (comment) |
yep looks good, build is working now on 7.4, 8.1, 8.2 |
tests are still failing, mainly because: |
Missing there as well: https://github.com/alexmontoanelli/php-radius/blob/master/tests/server/fake_server.php |
Maybe try the attribute.php and vsa.php here: https://git-lju.6connect.com/open-source/php-radius/-/tree/php8/tests/server |
grabbed the missing file from the 1.4b branch, however one test is failing, many are skipped, though I have all the required extensions installed:
|
I can confirm that now compiled correctly on:
But I got the same errors when make test:
|
Are you both sure that the sockets and pcntl functions are loaded? Try editing Or try On Windows I cannot load the pcntl extension, so a lot of tests fail if I remove |
Yes, they are OK:
|
Is the sockets extension compiled statically into PHP? Or do you have to load it in your php.ini? Looks like 20-sockets.ini loads it as shared extension. If it really is a shared extension make sure that you use a php.ini in your ‘make test’ that loads it. At least one of those checks on extension_loaded() in https://github.com/LawnGnome/php-radius/blob/master/tests/server/fake_server.php#L325 must return false and cause the SKIP. Try removing the check on sockets or pcntl. |
When running the tests, the sockets and pcntl extensions will not be loaded, it seams the testing environment is using a different php config. No idea how to fix this.
|
I get the same results from |
Try adding this to a *.phpt test script
See the tests for pcntl: https://github.com/php/php-src/blob/master/ext/pcntl/tests/001.phpt
|
Is any one here to solve this problem...? |
Does not build as-is for php80. Removing all occurrences of
TSRMLS_C
,TSRMLS_CC
,TSRMLS_DC
, andTSRMLS_FETCH
allows it to compile, but then every invocation of PHP spews a 'Missing arginfo' message for every defined radius function.The text was updated successfully, but these errors were encountered: