Reduce risk of segmentation fault in visit log #15471
Conversation
| @@ -274,18 +283,20 @@ public function render() | |||
| // can fail, for example at installation (no plugin loaded yet) | |||
| } | |||
|
|
|||
| ProxyHttp::overrideCacheControlHeaders('no-store'); | |||
| if ($this->sendHeadersWhenRendering) { | |||
There was a problem hiding this comment.
Wouldn't it make sense to refactor that some more and move sending headers away from the view render?
Imho it's a bit misplaced here and should be part of the global dispatch 🤔
There was a problem hiding this comment.
Yeah it just be a bit of a big refactoring potentially. I did have a different patch as well where it was detecting it more automatically but it's actually not that easy.
Eg I had a patch where
- view->render is called
- lock overwriting headers
- actually render the template
- no headers are sent while other views are being rendered as part of a postEvent in twig
- rendering template finished
- Remove the lock for overwriting headers
- Send headers
- view-> render is finished
It didn't quite work though because some views might still be rendered separately that aren't part of a postEvent.
We could generally do a global refactoring to get it out of view->render but sounds quite complex and not sure it's worth the effort also considering I haven't heard anyone else experiencing a seg fault in this direction.
I suppose we could put it into Controller::renderTemplate method but everyone not using that method would then suddenly need to remember to take care of sending the correct headers themselves? It may work though when doing it inside setBasicVariablesView/setGeneralVariablesView as most views would call this manually. It be a slightly better solution but also not perfect.
Not sure what you had in mind?
There was a problem hiding this comment.
yes, actually it would be nice to have a more clean response handling, but that would be a quite bigger refactoring. Guess for now your solution should be fine, as it solves the issue...
mattab
added
the
not-in-changelog
fixes #15307
This way I can view the visitor log. I can only view up to 25 visitors currently and it crashes when viewing 50. Once I change Custom Reports and Marketing Campaigns reporting I can view 500 visits without a crash.