Skip to content

feat: add a runtime for FrankenPHP with Symfony #124

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

Merged
merged 5 commits into from
Oct 15, 2022

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Oct 14, 2022

Add a runtime for the worker mode of FrankenPHP. To be released this afternoon at ForumPHP!

frankenphp

Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

small copy & paste error. Looking forward to Early Hints 👀

if ($this->kernel instanceof TerminableInterface && $sfRequest && $sfResponse) {
$this->kernel->terminate($sfRequest, $sfResponse);
}
} while ($ret);

Choose a reason for hiding this comment

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

Sorry for the review but maybe the callable could define its return type : void if I'm right, same for the first one:

$server = array_filter($_SERVER, static fn (string $key): bool => !str_starts_with($key, 'HTTP_'), ARRAY_FILTER_USE_KEY);

I'm not sure if the current lib tend to define return types on callable 🙁

Copy link
Member

Choose a reason for hiding this comment

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

I usually dont add return types on callables. It gives zero to little benefit. But it is just a matter of taste. As long as CI is not complaining, Im fine with adding return types.

Comment on lines +37 to +39
if ($this->kernel instanceof TerminableInterface && $sfRequest && $sfResponse) {
$this->kernel->terminate($sfRequest, $sfResponse);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should this part not be also inside the frankenphp_handle_request. Or does it have no difference here? If its inside or outside the callable. I'm not sure if the terminate is this way called for every request as it should, else I know some projects which would crash into unexpected state.

Copy link
Member

Choose a reason for hiding this comment

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

Frankenphp only cares about putting out the response. A "cleanup" or background process could be running outside IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if the terminate is this way called for every request as it should

It does.

@Nyholm
Copy link
Member

Nyholm commented Oct 15, 2022

Could you please rebase your PR now when #125 is merged?

@dunglas dunglas force-pushed the feat/frankenphp-symfony branch from 8f9b802 to 94136bc Compare October 15, 2022 13:12
@dunglas
Copy link
Contributor Author

dunglas commented Oct 15, 2022

@Nyholm rebased

@Nyholm
Copy link
Member

Nyholm commented Oct 15, 2022

Thank you!

@Nyholm Nyholm merged commit bc27ddc into php-runtime:main Oct 15, 2022
@Nyholm
Copy link
Member

Nyholm commented Oct 15, 2022

@dunglas dunglas deleted the feat/frankenphp-symfony branch October 15, 2022 15:59
@dunglas
Copy link
Contributor Author

dunglas commented Oct 15, 2022

Thank you very much @Nyholm!

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.

4 participants