Skip to content

Conversation

@crazywhalecc
Copy link
Owner

@crazywhalecc crazywhalecc commented Jun 19, 2025

What does this PR do?

Revert build-embed option as boolean. Related: php/frankenphp#1651 (comment)

This should not be a bug, but this would be misleading. Although docopt requires options to support space reading, it will cause some confusion according to our habits, especially in other SAPIs --build-XXX does not accept values.

But I think it is acceptable to use another option to override the default environment variable (such as --with-embed-type=shared, if you still feel it is necessary to override it with the command.

Otherwise, we consider it a bad practice to encourage others to put all parameters after the extensions.

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:
    • PHP_CS_FIXER_IGNORE_ENV=1 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 crazywhalecc requested a review from henderkes June 19, 2025 18:03
@henderkes
Copy link
Collaborator

A --with-embed-type=static|shared option would be a good compromise. I'll go back to the drawing board to figure out a better alternative.

@henderkes henderkes merged commit f3f581f into main Jun 20, 2025
13 checks passed
@henderkes henderkes deleted the revert/embed-option branch June 20, 2025 00:33
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.

3 participants