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

feat: add CLI support #239

Merged
merged 8 commits into from
Oct 9, 2023
Merged

feat: add CLI support #239

merged 8 commits into from
Oct 9, 2023

Conversation

dunglas
Copy link
Owner

@dunglas dunglas commented Oct 4, 2023

This patch allows FrankenPHP to execute CLI scripts. This is especially useful when using the static binary to run Composer, Symfony or Laravel commands.

Continuation of #4.

@dunglas dunglas force-pushed the feat/cli branch 2 times, most recently from ebf1585 to 293f57f Compare October 5, 2023 22:22
@dunglas
Copy link
Owner Author

dunglas commented Oct 7, 2023

This command is now good enough to run Composer and scripts built with Symfony Console!

frankenphp.c Outdated Show resolved Hide resolved
Comment on lines +34 to +36
os.Exit(status)

return status, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. This return will never be reached. I think returning will let Caddy call os.Exit anyway, so I don't think you need to do that yourself.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This doesn't work (the returned status code is always 1 in case of error, even if set explicitly in PHP with exit).

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 then that's possibly a bug in Caddy. Good to know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened caddyserver/caddy#5874 to fix that. The status code wasn't being carried through since we enabled Cobra

Comment on lines +29 to +31
if len(args) < 1 {
return 1, errors.New("the path to the PHP script is required")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me think, is there value in making php-cli -i work for example? (Could be useful to check enabled features). Some people use php -a and php -l on occasion too. But 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be great if ./frankenphp php <args> behaved identical to php <args> with php-cli installed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That would be nice, but it's a bit more work. The best option would be to allow the embedding of the official PHP SAPI (this requires a patch on PHP). I suggest merging this patch as is and trying to improve it later.

Co-authored-by: Francis Lavoie <[email protected]>
@dunglas dunglas merged commit c615fe0 into main Oct 9, 2023
25 of 27 checks passed
@dunglas dunglas deleted the feat/cli branch October 9, 2023 12:38
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