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

Proposal how to fix TODO #5

Open
Toflar opened this issue Apr 22, 2021 · 2 comments
Open

Proposal how to fix TODO #5

Toflar opened this issue Apr 22, 2021 · 2 comments

Comments

@Toflar
Copy link

Toflar commented Apr 22, 2021

I'm don't know why exactly you have to stop script execution here:

header('Content-Type: application/json');

But I assume there are technical reasons for it. I've encountered the same in the past and I've solved it by throwing a custom exception which you can then catch in the controller.
Not aborting the script execution has a few advantages:

  • You don't miss setting the Content-Length header like it is now ;-)
  • You can continue the script execution flow which means events etc. can still be fired after that before e.g. the response is sent to the client
  • Debugging scripts can continue to work
  • etc.

Just as an idea ;-) Over at Contao we even built a ResponseException which takes a Symfony HttpFoundation Response as argument so you can send a proper response from within legacy code :-)

@Sebobo
Copy link
Owner

Sebobo commented Apr 22, 2021

Hi @Toflar,

thanks for the idea!

Most of the time the command would work fine without early exit. But when flushing the Flow framework caches including reflection classes I encountered random exceptions during the shutdown phase of the framework.
Basically code was gone it still relied on.

So going back to the controller with an exception doesn't solve it or I didn't fully understand what you meant.

But after playing around it looks like the errors I encountered came from the persistence layer. So I tried clearing the persistence state after flushing, and so far I didn't encounter further errors when flushing all caches and returning a proper response.

So I will play with it for a few days and see if it actually works now :)
If not, maybe I can further influence the shutdown phase and skip some steps.

But I'm curious how you found this little todo?

Sebobo added a commit that referenced this issue Apr 22, 2021
The exception seemed to happen during persistence shutdown
so clearing its state seemed to solve it.

Resolves: #5
@Toflar
Copy link
Author

Toflar commented Apr 22, 2021

But I'm curious how you found this little todo?

I guess I'm trying to take my Neos Award jury seat seriously ;-) That's how I found it and I thought I might as well drop a few lines while I'm here :-)

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

No branches or pull requests

2 participants