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

Updated constructor params #1174

Closed
wants to merge 3 commits into from
Closed

Updated constructor params #1174

wants to merge 3 commits into from

Conversation

raoulduke
Copy link
Contributor

@raoulduke raoulduke commented Jun 25, 2018

Resolves #1173

Added ability to pass in host parameter. This is needed for new Logentries accounts which are now on a different domain (rapid7.com).

Note: You also need to pass in $useSSL as false because it seems port 443 is not an option on the new domain, but I left it in for backwards compatibility.

Ability to pass in host, needed for new Logentries accounts which are now on a different domain.
@@ -31,13 +31,13 @@ class LogEntriesHandler extends SocketHandler
*
* @throws MissingExtensionException If SSL encryption is set to true and OpenSSL is missing
*/
public function __construct($token, $useSSL = true, $level = Logger::DEBUG, $bubble = true)
public function __construct($token, $host = 'data.logentries.com', $useSSL = true, $level = Logger::DEBUG, $bubble = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go at the end of the parameter list or it will be a BC break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I'm using Laravel which automagically passes in the constructor params so I did not catch that.

@helhum
Copy link

helhum commented Jul 1, 2018

Looks good to me now

@Seldaek Seldaek closed this in dd144f7 Nov 4, 2018
@Seldaek
Copy link
Owner

Seldaek commented Nov 4, 2018

Thanks, merged in 1.x and master.

@Seldaek
Copy link
Owner

Seldaek commented Nov 4, 2018

Note that the correct way to do this is to migrate to the InsightOpsHandler

@lyrixx lyrixx mentioned this pull request Aug 16, 2022
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.

4 participants