8000 [Notifier] Custom Chatter instances have a *VERY* unexpected behaviour · Issue #41208 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Notifier] Custom Chatter instances have a *VERY* unexpected behaviour #41208

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.

Sign up for GitHub

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

Jump to bottom
Closed
wucdbm opened this issue May 13, 2021 · 8 comments
Closed

[Notifier] Custom Chatter instances have a *VERY* unexpected behaviour #41208

wucdbm opened this issue May 13, 2021 · 8 comments
Labels
Bug Notifier Status: Needs Review

Comments

@wucdbm
Copy link
Contributor
wucdbm commented May 13, 2021

Symfony version(s) affected: 5.2.4

Description

When configuring custom Chatter instances like so

        <service class="App\Core\Notifier\Slack\TransportFactory" id="core.notifier.slack.factory.my_app_name">
            <argument>%env(SLACK_CUSTOM_DSN_OTHER_THAN_DEFAULT_FRAMEWORK_CONFIG_ONE_WITH_DIFFERENT_CHANNEL)%</argument>
            <argument type="service" id="notifier.transport_factory.slack" />
        </service>
        
        <service class="Symfony\Component\Notifier\Bridge\Slack\SlackTransport" id="notifier.transport.slack.my_app_name">
            <factory service="core.notifier.slack.factory.my_app_name" method="create" />
        </service>
        
        <service class="Symfony\Component\Notifier\Chatter" id="notifier.chatter.slack.my_app_name">
            <argument type="service" id="notifier.transport.slack.my_app_name" />
        </service>

The problem is this code within the Chatter

public function send(MessageInterface $message): ?SentMessage
    {
        if (null === $this->bus) {
            $this->transport->send($message);

            return null;
        }

And the fact that its $bus argument gets autowired as my configuration has autowiring enabled in defaults.
I had to add autowire="false" to my service, which is quite un-intuitive given I was using a custom setup of the component for the first time.

How to reproduce

Code above should be enough to reproduce or understand the issue.

Possible Solution

I am very unaware what the purpose of that bus is, besides possibly delaying the messages until after the request has been served?
In either case, it looks like a design flaw to me. Why not decorate the transport with a service that depends on the Bus implementation?, so rather than directly sending through the transport, messages are sent to the bus first, and then through the transport, somehow.

Anyhow, I was only attempting to send a message to a different channel in Slack, so we're probably looking at the wrong barn.

I think we should focus efforts on figuring out why setting a recipieng other than the default channel in slack won't work, then this is a no-issue as far as one is only attempting to send messages to another channel.
If however somebody needs to send messages to several Slack (or any other service?) apps, it gets ugly again, because once the Bus sneaks in, everything is sent through the default Chatter instance.

Additional context

My goal was to be able to send messages to separate channels. The problem is, setting the recipient @ the MessageOptionsInterface instance (in my case, SlackOptions) did not work. I have investigated to the point where calling $message->getRecipientId() in doSend of SlackTransport returns NULL regardless of what I do, when using either of the transports: The default one defined by the framework, or my custom instances.

At present, my services are configured with an explicit autowire="false" to avoid injecting the Bus, and they work because I use a different DSN for each, which has the channel defined in the DSN. Even so, trying to send to another channel (for testing the sake of testing this issue) via MY OWN SERVICE ID, which avoids the Bus thingy, configuring another channel in the message's options still won't work. I did not investigate further why that is.
@wucdbm wucdbm added the Bug label May 13, 2021
@OskarStark OskarStark changed the title [Chatter] Custom Chatter instances have a *VERY* unexpected behaviour [Notifier] Custom Chatter instances have a *VERY* unexpected behaviour May 13, 2021
@OskarStark
Copy link
Contributor

Hi exactly the code snippet you are complaining about received a fix in #40982

Can you please update notifier to the latest release and check again?

In Symfony 5.3 we also added a feature to be able to disable the message bus. Its a PR made by @jschaedl

Hope this helps

@jschaedl
Copy link
Contributor

@wucdbm You could try out #39353 and check if this fixes your problem.

@OskarStark #39353 is not merged yet unfortunately.

@OskarStark
Copy link
Contributor

You are right it will be released in 5.4

@wucdbm
Copy link
Contributor Author
wucdbm commented May 13, 2021

#40982 Does not solve my issue, it being that messages (by default) are channeled through the bus, not the transport, which afterwards uses the default bus, rather than my transport OR being able to configure an alternative channel to send a message to on a per-message basis

#39353 looks like this will help - I can then configure the message bus to something invalid and force null in the Chatter
Assuming that then all messages are sent immediately, this will likely solve my issue

I am only unsure about the recipient message config not working properly - I guess I have to test using the framework's chatter and hardcode null. If it still doesn't send to the appropriate channel, then it won't help

@OskarStark
Copy link
Contributor

@fabpot as you are the inventor of this component, does this sounds familiar to you?

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

@wucdbm
Copy link
Contributor Author
wucdbm commented Dec 10, 2021

Could very well still be relevant, we did workaround this.

@carsonbot carsonbot removed the Stalled label Dec 10, 2021
@wucdbm wucdbm closed this as completed Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Notifier Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@OskarStark @wucdbm @jschaedl @xabbuh @carsonbot
0