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

Configuring the subscription broadcast name #1300

Closed
stayallive opened this issue Apr 18, 2020 · 0 comments · Fixed by #1301
Closed

Configuring the subscription broadcast name #1300

stayallive opened this issue Apr 18, 2020 · 0 comments · Fixed by #1301
Labels
discussion Requires input from multiple people enhancement A feature or improvement

Comments

@stayallive
Copy link
Collaborator

stayallive commented Apr 18, 2020

So right now it's "impossible" (nothing is of course) to configure the queue on which the subscription is broadcasted (if you choose to use the queue).

This is because an event/listener is used, because Laravel handles reading the $queue property very poorly and you cannot set it in the constructor or implement a getter method (bit more context).

There are a few options I see and I wanted to discuss it before creating a PR. Hope to discuss the solutions below and/or read better options.


1. Remove the use of a event/listener

And use a proper queued job on which we can run onQueue and use other methods to set the queue name read from the config file.

2. Read the event listener class from the container

Use an interface to read the event listener implementation from the container so the user can implement it's own (extend the current one and set the $queue property).

3. Use a "hack"

Defining this function on the event listener would work:

    public function __get($name)
    {
        if ($name === 'queue') {
            return env('QUEUE_NAME_AUDIO');
        }

        return $this->{$name};
    }

I'm partial to 1 because it allows us to just add a config option to set the queue name and use an actual queue job instead of queued event allowing us to set tags for Horizon too (see #1301).

But 2 sounds the most simple from a code perspective seeing as everything uses an interface already.

However 1 is probably a breaking change (removing the event) although we could trigger the new job from the event listener and remove the ShouldQueue from the event listener to solve that "issue" and add it to the 4.12 release.

3 is possible but seems like a terrible plan 😄

@stayallive stayallive added enhancement A feature or improvement discussion Requires input from multiple people labels Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Requires input from multiple people enhancement A feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant