Add php support #3
Conversation
| end | ||
|
|
||
| end | ||
| rescue *EXCEPTIONS |
There was a problem hiding this comment.
Is the intention really to quietly ignore these?
There was a problem hiding this comment.
I've reused the code from the old platform and not removing the rescue seemed to be a good idea. In the past, the rescue block was used to store values like timestamp on Statsd object, which at the moment is not included in the project.
There was a problem hiding this comment.
Gotcha. The situation is slightly different in engines land:
The engine itself shouldn't be concerned with exception handling (unless there are expected/ignorable exceptions) or timeouts. The application that's running the engine will handle all of that.
In short, I think you can remove this rescue and any timeout-related logic.
|
@pbrisbin Is this good to merge or should I open a new PR with the |
|
@pbrisbin @BlakeWilliams I've removed the rescue/timeout blocks in the PHP module. Please note that the same logic resides in Javascript module as well but I think it should be removed in a separated PR. |
|
@pbrisbin Merging this unless you have any objections. @jacobi007 Want to squash or is this good to go? |
|
@BlakeWilliams fine with me. To me, new engine PRs arent' super valuable to review closely because it's so much green. As long as they go through appropriate QA, I'd rather merge it and focus any code review on the bugfix PRs that come out after. |
|
Sounds good, I'll get this merged in soon then. |
Confirmed via this helpful report that we're mis-referencing php's nan. stderr gets flooded with notices. With this change, the notices go away and everything seems to work. #186 (comment)
No description provided.