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

Added Emoji support for the Slack Handler #413

Merged
merged 4 commits into from
Sep 29, 2014

Conversation

fotomerchant
Copy link

Added emoji support as per slack api doc: https://api.slack.com/methods/chat.postMessage. Updated tests.

@xabbuh
Copy link
Contributor

xabbuh commented Aug 22, 2014

You should at the new argument to the end to be backward compatible. And wouldn't be using null by default being better to keep the current behaviour? Thus, you would have to pass an icon explicitly.

@fotomerchant
Copy link
Author

True @xabbuh ... Will do that now

@fotomerchant
Copy link
Author

@xabbuh, take a look and let me know if there's anything else I need to attend to.

@xabbuh
Copy link
Contributor

xabbuh commented Aug 22, 2014

I wouldn't modify the existing test, but keep it instead to ensure that the old behaviour doesn't break. You can add a new for the icon.

@fotomerchant
Copy link
Author

Thanks for the assitance @xabbuh. Hope this is good to go.

@@ -54,7 +54,17 @@ public function testWriteContent()
fseek($this->res, 0);
$content = fread($this->res, 1024);

$this->assertRegexp('/token=myToken&channel=channel1&username=Monolog&text=&attachments=.*$/', $content);
$this->assertRegexp('/token=myToken&channel=channel1&username=Monolog&text=&attachments=.*/', $content);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this modification really needed? I would have expected the url not to change at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, if it is anchored at the end, it will not match anymore when &icon_emoji=%3Aalien%3A is added at the end

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is the test without the icon, isn't it?

@fotomerchant
Copy link
Author

OK. @stof @xabbuh I've re-added the regex string terminator to the original test (got left out accidentally). My new test takes care of the icon_emoji scenario and the code should have no BC breaks. Hopefully it's good to go!

@xabbuh
Copy link
Contributor

xabbuh commented Aug 26, 2014

To me it looks good now.

@Seldaek
Copy link
Owner

Seldaek commented Sep 29, 2014

Thanks everyone, looks good!

Seldaek added a commit that referenced this pull request Sep 29, 2014
Added Emoji support for the Slack Handler
@Seldaek Seldaek merged commit 4f85e55 into Seldaek:master Sep 29, 2014
@Seldaek
Copy link
Owner

Seldaek commented Sep 30, 2014

Just FYI I moved the emoji arg back before $level since SlackHandler was actually never released in a stable version of Monolog. Anyone depending on it should watch out.

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.

None yet

4 participants