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

Add sticky channel option #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

soullivaneuh
Copy link

Closes #40

With this option, all message will be sent to the default channel only.

This is useful for dev environment when you want to concentrate all test message to a specific test channel.

This was dicussed for a long time on #40 but we finally get an agreement with @Gummibeer at #40 (comment) 😉

@maknz Could you please review the code and merge it if it looks OK? Thanks.

@soullivaneuh
Copy link
Author

Added a note on readme.

With this option, all message will be sent to the default channel only.

This is useful for dev environment when you want to concentrate all test message to a specific test channel.
@Fabiencdp
Copy link

  • 1 !!

Copy link

@Gummibeer Gummibeer left a comment

Choose a reason for hiding this comment

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

Except the single boolean cast everything is ok.

*/
public function isStickyChannel()
{
return $this->sticky_channel;

Choose a reason for hiding this comment

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

This will force the result to be an boolean. Otherwise it can also be any other type.
return (bool) $this->sticky_channel;

Copy link
Author

Choose a reason for hiding this comment

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

According to the phpdoc, sticky_channel must be a boolean and developpers should respect this.

Casting anything to a boolean is not the right way IMO.

When this project will use PHP >=7.0, then we will be able to use strict type instead.

@soullivaneuh
Copy link
Author

Any news about this MR? Thanks.

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