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

[11.x] Accepts BackedEnum for onQueue, onConnection, allOnQueue, and allOnConnection methods in the Queueable trait #52604

Merged

Conversation

sethsandaru
Copy link
Contributor

Hi guys,

I believe there are a lot of applications out there that are maintaining the Queue Names & Connections using Enums, which is awesome and less hardcoded string.

This PR enables us to pass an Enum instance to these methods:

  • onQueue
  • onConnection
  • allOnQueue
  • allOnConnection

This shouldn't bring any breaking changes. Tests are covered everything ✌️

Note: this only enhances those listed methods above, for jobs that use $connection & $queue properties, they still need to use a normal string (or enum->value)

Before

image

After

image

@morloderex
Copy link
Contributor

Why not just pull out the BackedEnum's value directly before passing it to the methods?

@sethsandaru
Copy link
Contributor Author

Why not just pull out the BackedEnum's value directly before passing it to the methods?

Because it's long and somewhat tedious always to do ->value?

There are multiple parts in the framework that support BackedEnum so it totally makes sense for Queue's stuff

@henzeb
Copy link
Contributor

henzeb commented Aug 30, 2024

why backed enums. why not also support non-backed enums?

@sethsandaru
Copy link
Contributor Author

why backed enums. why not also support non-backed enums?

It's way more flexible than the $name of UnitEnum

@henzeb
Copy link
Contributor

henzeb commented Aug 30, 2024

why backed enums. why not also support non-backed enums?

It's way more flexible than the $name of UnitEnum

It's not flexible at all. With a backed enum I am basically write the name twice. case Default='default'.

@cosmastech
Copy link
Contributor

We do something like this in our application. We actually even went a step further and created a mapping between QueueConnection and Queue.

First we have a QueueConnection for connection names.

enum QueueConnection: string
{
    case SHORT = 'short';
    case LONG = 'long';
}
enum QueueName: string
{
    case ANALYTICS = 'analytics';
    case NOTIFICATIONS = 'notifications';
    case LONG_JOBS = 'long_jobs';

    public function getQueueConnection(): ?QueueConnection
    {
        return match($this) {
            QueueName::NOTIFICATIONS => QueueConnection::SHORT,
            QueueName::ANALYTICS, QueueName::LONG_JOBS => QueueConnection::LONG,
            default => null // just for illustration
        };
    }
}

And then added a method inside of our jobs like this:

    /**
     * @param  string|QueueEnum|null  $queue
     * @return $this
     */
    public function onQueue($queue)
    {
        if ($queue instanceof QueueEnum) {
            if ($queueConnection = $queue->getQueueConnection()) {
                $this->onConnection($queueConnection);
            }

            $queue = $queue->value;
        }

        $this->queue = $queue;

        return $this;
    }

Now inside of our Jobs, we only have to specify the queue and we get the connection along with it.

class SomeJob /* pretend all of the extends and use statements are here */
{
    public function __construct($param1, $param2) {
     $this->onQueue(QueueName::LONG_JOB);
   }
}

@taylorotwell taylorotwell merged commit 811ea6a into laravel:11.x Aug 31, 2024
28 of 29 checks passed
@christophrumpel
Copy link
Contributor

Hey @sethsandaru. Nice additon 👍

Just tried this out and it was not working as I expected it.

If start with "dispatch" and then chain "onConnection" it does NOT work to use a BackendEnum, because then the code is referring to a PendingDispatch class which does not yet support that.

At least this is what my IDE tells me. Am I missing something?

CleanShot 2024-09-11 at 16 26 27@2x

@sethsandaru sethsandaru deleted the 11.x-patch-accepts-stringbackedenum branch September 11, 2024 14:35
@sethsandaru
Copy link
Contributor Author

sethsandaru commented Sep 11, 2024

@christophrumpel Thanks,

Yeah I forgot to add the BackendEnum to PendingDispatch 😅, happy to add that. Since PendingDispatch will invoke the onQueue of Queueable, it should be fine.

My usecase: my Jobs are interacting with Queueable trait, I'm managing the Queue Names & Connections under Job's constructor which is working fine!

image
Off-topic

I'd suggest assigning the queue's name & connection inside the Job itself, outer layer shouldn't really determine that (just like the viaQueue viaConnection of Event Listener)

@christophrumpel
Copy link
Contributor

Ah yeah, I see that now. I will also use it like that in my video, thanks 👍

@sethsandaru
Copy link
Contributor Author

sethsandaru commented Sep 12, 2024

cc @christophrumpel PendingDispatch is updated (ref #52739), let's wait for the next release ✌️

Edit: looks like it is included in the latest release today 🚀

@christophrumpel
Copy link
Contributor

great 🙌 thanks

@FrittenKeeZ
Copy link

@sethsandaru It's also missing from Illuminate\Bus\PendingBatch

Illuminate\Bus\PendingBatch::onQueue(): Argument #1 ($queue) must be of type string, App\Enums\Queue given

@sethsandaru
Copy link
Contributor Author

sethsandaru commented Sep 30, 2024

@FrittenKeeZ yeah this PR was mainly focused on normal jobs, batch is not supported. Perhaps something we can add 😉

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.

7 participants