Skip to content

Conversation

crazywhalecc
Copy link
Owner

What does this PR do?

Fix #888

Checklist before merging

If your PR involves the changes mentioned below and completed the action, please tick the corresponding option.
If a modification is not involved, please skip it directly.

  • If you modified *.php or *.json, run them locally to ensure your changes are valid:
    • composer cs-fix
    • composer analyse
    • composer test
    • bin/spc dev:sort-config
  • If it's an extension or dependency update, please ensure the following:
    • Add your test combination to src/globals/test-extensions.php.
    • If adding new or fixing bugs, add commit message containing extension test or test extensions to trigger full test suite.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Sep 7, 2025

Looks like this function has not been modified 15 years. I don't use CGI but it looks good to merge in php-src. There's no reason not to use inline methods when possible, but I'm not quite sure if this will affect other things, and the official obviously won't apply this update for old PHP version.... So I think we could fix it in SPC individually first. This problem also only occurs in linux full-static builds.

@henderkes
Copy link
Collaborator

We can have the patch first, but this should definitely be submitted to php-src. Static musl is likely just not in their CI so they never noticed.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Sep 7, 2025

php/php-src#19746

If accepted in PHP 8.5, we could add version check later. patchFile can ignore patching if contents are totally identical.

@henderkes
Copy link
Collaborator

That would be a bug fix that would be backported to 8.1+ I think. But it seems like undefined references cause the builds to fail.

@crazywhalecc
Copy link
Owner Author

crazywhalecc commented Sep 7, 2025

Seems this change need to let other SAPI depend on cli SAPI if build statically or inline build. I just tested it with the single cli so I didn't find this issues. Maybe it can only be patched with the cli if we don't want to change dependencies.

@crazywhalecc
Copy link
Owner Author

Yikes. Keep this PR for now until a better solution is found.

@henderkes
Copy link
Collaborator

I'm not a fan of merging it like this. Let's wait what feedback we get from php-src.

@crazywhalecc crazywhalecc marked this pull request as draft September 18, 2025 11:15
@henderkes henderkes marked this pull request as ready for review October 8, 2025 09:34
Copy link
Collaborator

@henderkes henderkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in lack of a response from php-src, we kinda have to do it ourselves temporarily

@henderkes henderkes merged commit e6408b4 into main Oct 8, 2025
10 checks passed
@henderkes henderkes deleted the fix/static-readline branch October 8, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Precompiled Musl PHP Binaries don't support php -a

2 participants