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 GelfFormatter/GelfHandler #287

Closed
wants to merge 1 commit into from

Conversation

bzikarsky
Copy link

Change implementation to use bzikarsky/gelf-php as discussed in Issue #258

Change implementation to use bzikarks/gelf-php
@Seldaek
Copy link
Owner

Seldaek commented Dec 26, 2013

Thanks! As said though I can't really merge this before 2.0, so it might be a little while.

@bzikarsky
Copy link
Author

I totally understand. :-)

if (!class_exists("Gelf\MessagePublisher") || !class_exists("Gelf\Message")) {
$this->markTestSkipped("mlehner/gelf-php not installed");
if (!class_exists('\Gelf\Publisher') || !class_exists('\Gelf\Message')) {
$this->markTestSkipped("bzikarsky/gelf-php not installed");
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong package name here

Copy link
Author

Choose a reason for hiding this comment

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

True. Will fix that. Thanks!

@bzikarsky
Copy link
Author

Well, this took more commits than it should have. Do you want me to squash?

@Seldaek
Copy link
Owner

Seldaek commented Dec 26, 2013

If you are done with everything sure feel free to squash it

@bzikarsky
Copy link
Author

squashed & pushed. :-)

@bzikarsky
Copy link
Author

Since this came up several times:

Monolog 2.0 seems to be far away, but people like to switch to graylog2/gelf-php. mlehner's fork is not actively developed anymore in favor of the official lib.

What do you think of those 2 backwards compatible changes?

  1. Drop the constructor typehint for IMessagePublisher in https://github.com/Seldaek/monolog/blob/master/src/Monolog/Handler/GelfHandler.php#L36 and do a runtime check for either Gelf\IMessagePublisher or Gelf\PublisherInterface
  2. Library-specific implementation of GelfMessageFormatter::format
    We can identify the Gelf-library in use by the existence of Gelf\IMessagePublisher vs Gelf\PublisherInterface
    https://github.com/mlehner/gelf-php/blob/master/src/Gelf/IMessagePublisher.php
    https://github.com/bzikarsky/gelf-php/blob/master/src/Gelf/PublisherInterface.php

/cc @h4cc

@bzikarsky
Copy link
Author

Addendum: 2. is unneccessary. The GelfMessageFormatter is compatible with both graylog2/gelf.php and mlehner/gelf-php.

So it's just a change to the constructor of the GelfHandler. To verify the functionality I would add an additional GelfHandlerLegacyTest class, which gets executed when mlehner's fork is installed.

@bzikarsky
Copy link
Author

I'll close this PR, and open a new one with an updated description.

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.

3 participants