Revert debugging socket changes to Bedrock Client #247
Conversation
This reverts all user changes to Client.php back to version 2.1.4 while preserving 4 commits by other developers: - cfb81c7 - neil-marcellini - Save commitCount when response says to - 7348afc - neil-marcellini - Fix phan - 9c6824f - neil-marcellini - Clean up comment per feedback - 28f307e - Thomas Wodarek - Replace deprecated TRAVIS envvars with GITHUB envvars Net change: -87 lines (removes user's debugging and socket handling changes)
| socket_getsockname($this->socket, $localAddress, $localPort); | ||
|
|
||
| if ($socketErrorCode === 115) { | ||
| $this->logger->info('Bedrock\Client - socket_connect returned error 115, waiting for connection to complete.', [ |
There was a problem hiding this comment.
I think we should keep this code, as
- It is still happening
- It is more correct than before your changes, and thus muuuuch less confusing to the next engineers who will look at it 😄
There was a problem hiding this comment.
ok putting that message back
There was a problem hiding this comment.
I don't get it, we are not waiting anymore, the previous message made more sense: Bedrock\Client - socket_connect returned error 115, continuing.
There was a problem hiding this comment.
Sorry for my lack of clarity. I think we should put all of the code doing the proper handling back.
There was a problem hiding this comment.
Ah ok so we want to leave this code as is then and not revert anything? I guess I can clean up a few things here though
| socket_getsockname($this->socket, $localAddress, $localPort); | ||
|
|
||
| if ($socketErrorCode === 115) { | ||
| $this->logger->info('Bedrock\Client - socket_connect returned error 115, waiting for connection to complete.', [ |
There was a problem hiding this comment.
I don't get it, we are not waiting anymore, the previous message made more sense: Bedrock\Client - socket_connect returned error 115, continuing.
Explanation of Change
Revert all changes I made for debugging https://github.com/Expensify/Expensify/issues/544983
This wasn't straight forward because of other people making changes to this file as well. I think this is right though, but double check.
Related Issues
https://github.com/Expensify/Expensify/issues/544983
Deployment