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

[job-queue] Change type hint from Closure to callable #286

Closed
msheakoski opened this issue Dec 1, 2017 · 3 comments · Fixed by #292
Closed

[job-queue] Change type hint from Closure to callable #286

msheakoski opened this issue Dec 1, 2017 · 3 comments · Fixed by #292

Comments

@msheakoski
Copy link
Contributor

In the JobRunner (and possibly other areas), the type hint currently limits you to working with closures. For more complex jobs, it is nicer to have an object because you can utilize DI and autoloading. Changing Closure to callable would maintain backwards compatibility and open up more options to the end user.

public function runUnique($ownerId, $name, \Closure $runCallback)

public function createDelayed($name, \Closure $startCallback)

public function runDelayed($jobId, \Closure $runCallback)

@msheakoski
Copy link
Contributor Author

I recently learned on http://php.net/manual/en/functions.anonymous.php

As of PHP 5.4.0, when declared in the context of a class, the current class is automatically bound to it, making $this available inside of the function's scope.

which means that the queue processor itself can utilize DI and more complex code can be broken down into class methods on the processor instance instead of putting all of the code inside of the closure.

class MyConsumer implements PsrProcessor
{
    protected $em;
    protected $jobRunner;

    public function __construct(
        EntityManagerInterface $em,
        JobRunner $jobRunner
    ) {
        $this->em = $em;
        $this->jobRunner = $jobRunner;
    }

    public function process(PsrMessage $message, PsrContext $session)
    {
        $id = $message->getBody();
        if (!preg_match('/\A\d+\z/', $id)) {
            return self::REJECT;
        }
        $result = $this->jobRunner->runUnique("job-$id", "myjob", function () use ($id) {
            return $this->doTheWork($id);
        });
        return $result ? self::ACK : self::REJECT;
    }

    protected function doTheWork(string $id): bool
    {
        $repo = $this->em->getRepository(MyEntity::class);
        /** @var MyEntity|null $entity */
        $entity = $repo->find($id);
        if (!$entity) {
            return false;
        }
        // ... more work ...
        return true;
    }
}

@makasim
Copy link
Member

makasim commented Dec 4, 2017

Could you open a PR?

@msheakoski
Copy link
Contributor Author

msheakoski commented Dec 4, 2017

The above example works without any changes. I was just sharing a workaround that I found for my initial problem after I learned that $this is accessible as the queue processor instance from inside the closure.

I think that callable is still better than Closure because it gives the end user more flexibility for how they want to design their queue processor, such as moving the logic to an external class or domain object. I need to look through the enqueue code to see if anything specifically depends on closures then I will submit a PR if the change is okay to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants