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

GELF: Allow for both the legacy and the updated official library #326

Merged
merged 3 commits into from
Feb 23, 2014

Conversation

bzikarsky
Copy link

Since #287 would have broken BC, this PR allows to use both the legacy GELF library (mlehner/gelf-php) and the official new one (graylog2/gelf-php) with Monolog.

Changes:

  • Removed typehint in GelfHandler and replaced it with a runtime check for either IMessagePublisher or PublisherInterface
  • The GelfHandlerTest is quite dependent on the used GELF library. That's why I moved the tests for the legacy library to a separate file GeldHandlerLegacyTest
  • Introduced a feature-switch in GelfFormatterTest for an easy test with both libraries
  • composer.json now defaults to graylog2/gelf-php for suggestion and require-dev. I cannot think of a way to automatically test for both the legacy and current library (naming clashes). To verify legacy-compatibility, one must manually change to mlehner/gelf-php: ~1.0, and run composer update && phpunit. Better idea are very welcome!

This enables users to switch to the current gelf-library by changing their composer.json to graylog2/gelf-php and does not change anything for users unwilling or unable to change. It should be able to introduce this PR in the next minor version.

/cc @h4cc

Benjamin Zikarsky added 3 commits December 28, 2013 18:57
@Seldaek Seldaek added this to the 1.8 milestone Feb 23, 2014
@Seldaek Seldaek mentioned this pull request Feb 23, 2014
30 tasks
Seldaek added a commit that referenced this pull request Feb 23, 2014
@Seldaek Seldaek merged commit b56ed7b into Seldaek:master Feb 23, 2014
@Seldaek
Copy link
Owner

Seldaek commented Feb 23, 2014

Thanks, that's a much preferable alternative

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.

2 participants