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

Stream only specific responses #18348

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cedric-anne
Copy link
Member

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.

Description

The proposed changes should permit to limit the usage of streamed responses to the endpoints that actually really require it.

These changes will also permit:

  • to detect unexpected flush()/ob_flush()/ob_clean()/ob_end_clean() operations (I added a warning for this);
  • to add the Symfony profiler in all GLPI pages, unless an exit()/die() instruction is used.

Whitespace changes should be ignored for the review. Many changes are just related to encapsulation of some scripts in a streamed response object.

@cedric-anne cedric-anne added this to the 11.0.0 milestone Nov 19, 2024
@cedric-anne cedric-anne self-assigned this Nov 19, 2024
Copy link
Contributor

@cconard96 cconard96 left a comment

Choose a reason for hiding this comment

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

This would break the handling of debug error messages by the new API.
See DebugResponseMiddleware.php.

This handling would have to be adjusted.

@cedric-anne
Copy link
Member Author

This would break the handling of debug error messages by the new API. See DebugResponseMiddleware.php.

This handling would have to be adjusted.

I reverted the changes made on the HLAPI. It will not be impacted by the current changes because it has its own controller.

Copy link
Member

@flegastelois flegastelois left a comment

Choose a reason for hiding this comment

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

Blank page (without errors): /front/ruledictionnarysoftware.php?replay_rule=replay_rule

@cedric-anne cedric-anne marked this pull request as draft November 19, 2024 13:14
@cedric-anne
Copy link
Member Author

cedric-anne commented Nov 19, 2024

I put this PR back to draft.

In GLPI 11.0, the progress bar display is broken. I am not able to understand what is the issue behind this. Anyway, using flush() not always work as expected. For instance, if there is a proxy between the client and the PHP server, then the proxy may wait for the full response to be received before sending it to the client. The webserver itself may act like this.

With #18296, we will add a new progress bar system that we will have to use in replacement of the current system. Once it will be done, the current PR will be ready to be merged.

I tested the /front/cron.php behaviour (by putting sleep(5); Toolbox::logDebug('OK'); after the flush() instruction), and it is OK on my machine, the browser consider the response as fully received after 50ms, and the execution continues on the PHP server.

@cedric-anne cedric-anne force-pushed the 11.0/streamed-responses branch from 8f2e565 to e4f9603 Compare November 19, 2024 14:50
- fix cron system in web context
- permit legacy scripts to return their own `Response` objects
- trigger error when content is flushed by a legacy script
- deprecate `Html::glpi_flush()`
- deprecate the legacy progress bar system
@cedric-anne cedric-anne force-pushed the 11.0/streamed-responses branch from e4f9603 to cb6a3aa Compare November 28, 2024 10:23
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.

5 participants