Skip to content

Add PHPDbg support to HTTP components #26749

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 1 commit into from
Apr 3, 2018
Merged

Add PHPDbg support to HTTP components #26749

merged 1 commit into from
Apr 3, 2018

Conversation

hkdobrev
Copy link
Contributor

@hkdobrev hkdobrev commented Apr 2, 2018

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This is a follow-up to #26677.

Turns out there aren't that many remaining instances of PHP_SAPI checks without considering phpdbg where it's needed.

@hkdobrev hkdobrev changed the title Add PHPDbg support to HTTP compoenents Add PHPDbg support to HTTP components Apr 2, 2018
Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.
I left a minor comment.

Did you check for all references to PHP_SAPI ?

@@ -372,7 +372,7 @@ public function send()

if (function_exists('fastcgi_finish_request')) {
fastcgi_finish_request();
} elseif ('cli' !== PHP_SAPI) {
} elseif (!in_array(PHP_SAPI, array('cli', 'phpdbg'))) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you update your code with } elseif (!\in_array(PHP_SAPI, array('cli', 'phpdbg'), true)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@hkdobrev
Copy link
Contributor Author

hkdobrev commented Apr 2, 2018

@lyrixx Thanks for the quick review! I did check every usage of PHP_SAPI and while there are a few others they already have either special support for phpdbg, similar check to that or are just about cli-server SAPI where phpdbg check is not applicable.

@hkdobrev
Copy link
Contributor Author

hkdobrev commented Apr 2, 2018

I've just unified the code style in all checks.

@hkdobrev
Copy link
Contributor Author

hkdobrev commented Apr 2, 2018

Important consideration is these would need to be re-checked in 2.8, 3.4, 4.0 etc.

@fabpot
Copy link
Member

fabpot commented Apr 3, 2018

Thank you @hkdobrev.

@fabpot fabpot merged commit 60dd79c into symfony:2.7 Apr 3, 2018
fabpot added a commit that referenced this pull request Apr 3, 2018
This PR was squashed before being merged into the 2.7 branch (closes #26749).

Discussion
----------

Add PHPDbg support to HTTP components

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

This is a follow-up to #26677.

Turns out there aren't that many remaining instances of `PHP_SAPI` checks without considering `phpdbg` where it's needed.

Commits
-------

60dd79c Add PHPDbg support to HTTP components
@hkdobrev hkdobrev deleted the phpdbg-support branch April 17, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants